Skip to content

chore(release): land lock-refactor integration branch on main (#351, #352, #362, #369, #364) - #404

Closed
Bnjoroge1 wants to merge 325 commits into
mainfrom
Bnjoroge/lock-refactor
Closed

Bnjoroge1 wants to merge 325 commits into
mainfrom
Bnjoroge/lock-refactor

Conversation

@Bnjoroge1

@Bnjoroge1 Bnjoroge1 commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Lands the outstanding work on the Bnjoroge/lock-refactor integration branch onto main.

Contents (branch-only commits, each already merged into the branch via its own PR)

CI fixes carried in the same range

  • test(server): add git_forge_url to the snapshot-rewrite fixture — the test-support build of preloop-runner-server did not compile after fix: route remaining hardcoded GitHub hosts through configured URLs (#349) #352 added the field; the fixture fix existed only on feat/check-run-outbox.
  • test(server): assert the PAT follows the configured origin over http — the old resolve_ref_to_sha_omits_pat_over_http contradicted fix(runner-server): attach static GitHub PAT by configured origin, not scheme #351's deliberate behavior (configured origin receives the PAT regardless of scheme). The unconfigured-origin half stays pinned by pat_targets_configured_github_regardless_of_scheme.
  • test(server): keep PAT scope introspection hermetic in test-support builds — cargo test shares one process env, so a PRELOOP_GITHUB_TOKEN from a neighbouring test (or the job VM) turned unrelated submits into a live 401 → 403 refusing to embed an invalid PAT. Test-support builds now treat the default GitHub API base as unverifiable unless a test points PRELOOP_GITHUB_API_URL at its own stub.

Divergence vs main before this PR

48 files, +1996 / −400 (the five PRs above plus the CI fixes).


Devin Review


Summary by cubic

Lands the control-plane rewrite and GitHub forge configuration work from the lock-refactor integration branch. The server now runs every scheduling command and hot API path as short conditional SQL transactions against a persisted schema (Postgres or SQLite) instead of loading and writing back the whole working set per request, and job messages are stored secret-free and filled only when a runner claims the job.

Refactors

  • The old in-memory TxState model is removed; control/pg and control/lite implement the shared ControlBackend trait with mirror-image SQL, and lite is the default backend.
  • Hot API paths (step reports, repository lookups, live-log job lists, check-run reporting, worker-token issuance) now execute as indexed statements instead of materializing TxState.
  • Submission secrets move into the SecretProvider's run tier at submit and are never persisted; job templates carry only names and a fill spec.
  • The static GitHub PAT follows the configured origin regardless of scheme, remaining hardcoded GitHub hosts route through configured URLs, and github.repositoryUrl advertises the configured forge.
  • Concurrency admission no longer self-admits a run's own jobs.
  • Load improvements resize writer/reader pools via env, batch claim candidates into one FOR UPDATE SKIP LOCKED read, and release workflow-level concurrency only on the run's final job completion.
  • A new control-plane workflow runs the backend suite against Postgres 16/17/18.

Bug Fixes

  • Test-support builds no longer introspect the default GitHub API over the network, so a leaked PRELOOP_GITHUB_TOKEN no longer turns unrelated submits into 401/403 failures.
  • Request ids are drawn from a Postgres sequence reserved per minting command, fixing duplicate ids across concurrent writers.
  • Concurrency.cancel-in-progress round-trips through serialization instead of silently dropping to None.
  • set_job_check_run now uses IS DISTINCT FROM instead of SQLite-only IS NOT, fixing Postgres webhook deliveries.
  • The webhook inbox module is now compiled, fixing a recursion crash on the first stats refresh.

Written for commit 53f1f15. Summary will update on new commits.

Review in cubic Turn on auto-fix

Replace TxState load/write-back on per-request reads with indexed
statements:

- report_steps: WorkflowStepsUpdate resolves the callback (plan then
  agent-job) and merges into job_steps rows in one transaction — an
  UPDATE that preserves stored name/runner_number/timestamps, then a
  guarded INSERT for undeclared steps. Terminal-only reports no longer
  invent started_at.
- attempt_repository / attempt_in_run: job-runtime-token repo lookup and
  snapshot Git-token membership become single joins; no working set.
- run_job_ids: the live-logs SSE job list no longer materializes TxState.
- run_held + project_run_data: get_run/get_public_run/run_events read
  run_record + manifests directly; the projection takes its held flag
  and step manifests as inputs.
- report_check_runs_for_run / clean-push check-run reporting read
  run_dispatch_info instead of a scoped load.
- issue_worker_token goes through issue_debug_token with the preserve
  check before the already-issued conflict; the reaper calls
  sweep_stale_bindings directly; the status mirror reads queue_stats.
- run_step_manifests qualifies joined columns (both backends); the
  ambiguity broke archived-manifest reads.
- next_message reverts to a plain transaction: broker-hybrid sessions
  (the load path) poll through poll_session/poll_claim_direct instead.

New shared suite test `step_reports_merge_into_manifest` covers merge
semantics on SQLite and Postgres.
…ck-run SQL

Request ids were minted in memory as max(request_id)+1. Run-scoped
writers on different runs hold different locks and overlap, so two
submits could mint the same id; the write-back upsert then kept the first
attempt's identity columns and the second attempt's message. That
runner's timeline PATCHes resolved to no request (403) and its
completion to no request (404): ~50 403/s and ~8 404/s under load.

Postgres now draws ids from request_id_seq, reserved up front by the two
commands that mint attempts (submit, expansion apply). A command that
mints more than it reserved fails its transaction instead of racing.

set_job_check_run used SQLite's `IS NOT $3`, a Postgres syntax error that
failed every webhook delivery; it now uses IS DISTINCT FROM with typed
parameters.

run-round.sh: empty-array expansion under bash 3.2 `set -u`, and the
integrity query now reads webhook_deliveries through search_path.

Tests: concurrent_submits_mint_distinct_request_ids (fails before the
fix: "lost its request row"), check_run_mapping on both backends.
Load round r15: 0 timeline 403s, 0 completion 404s, 1566/1566
acquired jobs completed, 0 queued after drain.
…-back

Load rounds at 10 runs/s (~15 jobs/s) plateaued at 6.3 completed jobs/s.
Postgres wait sampling and per-caller transaction stats showed why:

- Writer pool: 4 connections per node capped in-flight writes; every
  claim/complete/submit queued for one. Pools are now sized by
  PRELOOP_PG_WRITERS / PRELOOP_PG_READERS (default 16 each) and opened in
  parallel.
- Claims: lock_poll_candidates walked the queue head one row at a time
  (two round trips per candidate, up to 256) and held a shared run lock
  on every run it passed, stalling completions of those runs. It now reads
  the head once, matches labels in Rust, locks the first free matches with
  one FOR UPDATE SKIP LOCKED statement, and takes shared run locks only
  on rows it claimed. Still never waits.
- Workflow-level concurrency: every job completion of a run with a
  `concurrency:` group took the global writer lock (~29 s each under
  load, 117 per round). run_in_concurrency now classifies runs
  (None / WorkflowOnly / Gated). A WorkflowOnly run's completion settles
  under its run lock and widens to the global scope (ControlError::
  WidenScope, rolled back and retried) only when it finishes the run,
  which is the only time a Holder::Run is released.
- Write-back: the request change signature Debug-formatted each full job
  message at load and at write, and changed rows re-sealed the unchanged
  message. The message is minted once with its request id and never
  changes in a command, so existing rows now update only their mutable
  columns and the signature excludes the message.

Harness integrity checks replace duplicate_runners (a runner that
re-registers under its name after a timeout is expected) with the real
invariants: names_running_twice and active_owner_mismatch.

Test: workflow_concurrency_releases_when_the_last_job_finishes (hold kept
after a non-final completion, released by the final one).

Same 10 runs/s round: completed 6.3 -> 9.9 jobs/s with the queue fully
drained; global completions 117 -> 7; claim txn 1.15 s -> 0.48 s.
…cisions

Migrate production callers outside control/ to backend trait methods:
- distributed_task::next_message -> poll_azdo_session + delivery-time
  render (acquire_context template + derived session key, no stored keys)
- distributed_task::complete_job_settling -> settle_job (the widen loop,
  session resolution, concurrency release all live in the backend)
- drop dead TxState helpers: build_task_agent_message,
  build_broker_plaintext_message, resolve_callback_job,
  sole_active_unfinished_request, job_request_tuple,
  next_broker_message_id, ensure_broker_request_owner (dup),
  latest_attempt_steps, project_run, live_log_key_for_job,
  live_log_run_terminal
- lib_tests: use backend live_log_key + control ensure_broker_request_owner

logic.rs decisions (coordinator review):
- aggregate_needs_status: non-terminal/missing -> None FIRST, matching the
  doc and GitHub (a failure among unfinished needs no longer aggregates)
- concurrency_admission: same-run no longer self-admits; with
  cancel_in_progress=false a different job of the same run waits on the
  group. Same-run exclusion only suppresses cancellation of the holder.
- add behavior tests for mixed terminal/nonterminal needs and same-run
  admission
…y, workflow_path)

Match the agreed workflow_run_numbers PK (namespace_id, repository,
workflow_path). The single-tenant deployment passes DEFAULT_NAMESPACE +
submission.repository + workflow_path; legacy backends fold
(repository, workflow_path) into their single-key counter so numbering
stays unique per repo+workflow.
…lement)

Port the scheduling state machine onto the agreed tables with no TxState and
no working set: every transition is one conditional statement on `jobs`
(`status` = workflow truth, `queue_state` = dispatch copy).

- lite/jobs.rs: row codecs (`jobs`/`job_specs`/`job_needs`/`job_messages`),
  `insert_job` + spec/needs/message writers, the run dependency graph
  (`RunGraph`, expanded `matrix_parent` nodes excluded while `reusable_caller`
  rows stay visible), `summarize_run_row`, outbox emission with `run_seq`,
  and `run_record` (runs + run_submissions + leaf jobs + spec joins).
- lite/concurrency.rs: `concurrency_holds`/`concurrency_waits`/`jobsets`/
  `jobset_gates` substrate. `acquire` runs the shared decisions
  (`logic::concurrency_admission`, `concurrency_queue_decision`) plus the two
  preemption rules from `try_acquire_concurrency` (stuck-holder displacement,
  stale-arrival supersession via `concurrency::event_order`).
- lite/promote.rs: `promote_run` — the `promote_ready_jobs` sweep over
  `queue_state = 'blocked'` rows, with `dependency_decision` on the graph
  view, job gates, caller/embedded JobSet gates, max-parallel, message
  hydration (`needs` context + deferred environment), and `on_job_enqueued`
  assignment intent over `job_assignments`/`provision_requests`.
- lite/settle.rs: `settle_node` (first-result-wins, continue-on-error,
  replayed completions, run re-summary), request retirement/purge, matrix
  fail-fast, cancellation (`cancel_run_inner`/`cancel_job_inner` incl.
  subtree), and concurrency release with FIFO promotion of the next waiter
  (max-parallel-saturated waiters re-park at their original `wait_id`).
- lite/submit.rs: `submit_run` — replay dedup on
  `(webhook_delivery_id, workflow_path)`, empty-group rejection, workflow
  gate, unhostable-platform check, per-job classification (held / blocked /
  ready / terminal), request minting with `requestId` patched into the stored
  message, token request + step manifest seeding, then the promotion sweep.

Verified: `cargo check -p preloop-runner-server --all-targets` clean;
`cargo test -p preloop-runner-server --lib control::lite` 3 passed.
…very

Compat/default sessions never do the key exchange, so rendering them
encrypted made cancellations undecodable (4 cancel tests). SessionMessage
now carries plaintext, set at queue time from the session maps and honored
by render_session_message; keyed broker/azdo sessions still encrypt with
the derived key.

Recovery tests no longer assert session_keys persistence (dead under the
derived-key contract): they assert the session runner binding survives.

The replay-counter test reads the folded repository\x1fpath key.
Bnjoroge1 and others added 21 commits October 3, 2026 23:30
…ed majors"

This reverts commit 66116cd.

Real-world workflows (deno, react) still pin actions declaring
runs.using: node20 and failed every such step. Restore the migration-era
behavior: node20 actions run (upgraded to node24 under
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 / actions.runner.usenode24bydefault),
with the node20 deprecation warning. Kept the later $/ self-reference
branch in run_action_from_dir and collapsed one nested if for clippy.
…ends

Suspended or deleted namespaces start no jobs; draining finishes queued
work. max_running_jobs and namespace_pool_limits cap concurrent claims
(a capped PG claim locks the limit row); jobs over a cap wait queued
without binding a warm runner. API/CLI submits require an active
namespace and are checked against max_queued_jobs and
submit_rate_per_minute (429) and max_jobs_per_run (400). Webhook runs
are never refused except a deleted namespace; they wait at claim.
Namespaces without limit rows behave as before.
A static-PAT job queued past the 5-minute scope-cache TTL was handed the
runtime token instead of the PAT, so its checkout failed with 'could not
read Username'. Acquire calls verified_pat_scopes, which re-introspects
on an expired entry; an unverifiable PAT stays withheld. The broker's
GenerateIdTokenUrl also pointed at the runner-facing run-service route;
it now matches the build-time message's /runner/server/ path with
api-version.
…campaign

Runner provisioning left /tmp on the guest tmpfs/overlayfs root, where
name_to_handle_at is unsupported: fanotify FID watchers failed (126 of
TypeScript's internal/fswatch tests). Bind /tmp to a directory on the
ext4 workspace disk, matching GitHub-hosted runners. The conformance
campaign can now pull the digest-pinned golden from ghcr (cached),
refuses a non-Postgres PRELOOP_STORE_URL so the lock refactor is
actually exercised, and PRELOOP_SKIP_GH_TOKEN runs tokenless.
Every isSecret variable on the wire lands in the runner's secrets context
(and toJSON(secrets) output). Stamping the full SecretProvider::resolve()
key set into spec.names shipped the whole scope to every job, referenced
or not. spec.names now carries merged ∩ referenced, collected statically
from the expanded plan:

- collect_secret_reads (preloop-gha-expressions): literal names plus a
  dynamic flag for secrets[expr], * filters, bare secrets paths, and
  unparseable expressions — those jobs keep the full scope (fail closed).
- collect_job_secret_reads (preloop-gha-parser): per-job walk covering
  env/if/steps/container/services/environment/outputs/name/concurrency
  and reusable secrets: maps; collect_secret_requirements rebased on it.
- inherit callees keep the caller scope; map callees unchanged; engine
  tokens (secrets.GITHUB_TOKEN) mint per claim as before.
- FillOutcome.masked keeps the node masker at full scope breadth so
  unreferenced values are still redacted server-side.

Divergence from github.com: toJSON(secrets)/secrets.* enumerate only
referenced names; documented in docs/fidelity-gap.md.
#369)

* ci(supply-chain): allow isolated dependency manifest and lockfile PRs

* chore(supply-chain): exempt pinned refinery migration crates

---------

Co-authored-by: Bnjoroge1 <Bnjoroge1@users.noreply.github.com>
`build_env` prepended the orchestrator-installed toolchain dirs
(`/home/runner/.cargo/bin`, `/home/runner/go/bin`) whenever they exist on
the host, which overrode a workflow-provided `env.PATH`. Hosted runners
replace the image PATH wholesale when a job/step sets PATH, so the shims
must only repair the machine-PATH fallback — the contract `ensure_path`
and `build_env_respects_explicit_job_path` were written with, before the
shim injection was added later.

Gate the injection on a non-empty job/step PATH and cover the gate with a
deterministic test that uses a temp dir as the shim (the guest paths do
not exist on dev hosts).

Fixes the rust shard 4 failure on #347:
worker::execution_context::tests::build_env_respects_explicit_job_path
(left: /home/runner/.cargo/bin:/custom/bin, right: /custom/bin).
Two tests merged from main still reached into inner.runs, which the lock
refactor moved into the control database: read via test_tx() and mutate
through test_db_mutate, matching the sibling test helpers.
…t scheme (#351)

* fix(runner-server): attach static PAT by configured GitHub origin, not scheme

The static GitHub PAT was attached to action-resolution and tarball
requests only when the outgoing URL started with `https://`. Pointing the
engine at a plain-http GitHub emulator (gh-simulate local mode) therefore
silently dropped the credential, and the anonymous API budget (60/hr) was
exhausted almost immediately.

Replace the scheme check with `url_targets_configured_github`, which
attaches the PAT when the request URL's host (and effective port) matches
one of the configured `github_urls` endpoints, regardless of scheme. Real
github.com still attaches over https; an unrelated host never receives
the credential.

Refs #349

* ci: rerun checks after shared engine recovery

The previous check run used the broken macstudio golden/provisioning state.
No source files changed.

* fix(ci): re-pin dtolnay/rust-toolchain in control-plane.yml

The pinned SHA (6bed0761) is not a commit in dtolnay/rust-toolchain;
zizmor flags it as impostor-commit. Re-pin to the real stable head
89b12181 (same pin ci.yml uses).

---------

Co-authored-by: Bnjoroge1 <Bnjoroge1@users.noreply.github.com>
…bes (#353)

* fix(runner): add off-switch for post-renew Actions service health probes

The four fire-and-forget probes fired after the first renewjob target real
GitHub service hosts (broker/run/results-receiver/token.actions.githubusercontent.com).
Against a hermetic GitHub emulator those hosts are unreachable, making the
probes guaranteed-fail noise with no connectivity value.

Add PRELOOP_DISABLE_ACTIONS_PROBES: when truthy (1/true/$true, matching the
official StringUtil.ConvertToBoolean vocabulary) the runner skips the four
probes and records no ConnectivityCheck telemetry, the absent equivalent.
Unset keeps official behavior identical.

Refs #349

* ci: rerun checks after shared engine recovery

The previous check run used the broken macstudio golden/provisioning state.
No source files changed.

* fix(ci): re-pin dtolnay/rust-toolchain in control-plane.yml

The pinned SHA (6bed0761) is not a commit in dtolnay/rust-toolchain;
zizmor flags it as impostor-commit. Re-pin to the real stable head
89b12181 (same pin ci.yml uses).

---------

Co-authored-by: Bnjoroge1 <Bnjoroge1@users.noreply.github.com>
…354)

* feat(runner-server): configurable CA roots for outbound GitHub HTTPS

Pointing the engine at a GHES/emulator forge that serves its own CA (e.g.
gh-simulate's generated CA) fails TLS verification on every server-side
GitHub call — the clients were built ad-hoc with only the bundled roots.

Add shared_http::ca_certificates() honoring PRELOOP_GITHUB_CA_FILE (forge-
scoped, additive) and SSL_CERT_FILE (the OpenSSL/native convention the
runner already honors in client::http::with_control). Apply via
github_client_builder() to the shared CLIENT plus the ad-hoc builders in
actions.rs, remote_workflows.rs, github_app.rs, and the snapshots LFS
forge client, so custom roots reach every GitHub-facing call uniformly.

Missing/unreadable/invalid bundles log a warning and add no roots —
verification stays on; startup never aborts on a bad CA path.

Refs #349

* style: format CA bundle helper

* ci: rerun checks after shared engine recovery

The previous check run used the broken macstudio golden/provisioning state.
No source files changed.

---------

Co-authored-by: Bnjoroge1 <Bnjoroge1@users.noreply.github.com>
…ates poll heartbeat UPDATEs on idle) (#362)

* Make idle broker polls reader-only and notify-driven

Idle long-polling runners were the dominant writer-pool load: every poll
opened a writer transaction, re-stamped last_seen_at, and re-ran the full
claim scan, and the root broker handler re-polled every 3 seconds while
holding the 50s request open.

- poll_session now probes on the reader pool first (session, oldest
  message, active request, pending cancellation, ready check) and only
  opens the writer transaction when there is a cancellation to deliver
  or a claim to attempt. Ownership is still revalidated inside the
  writer transaction, and a probe hit that loses a race just comes back
  Empty, as before.
- next_message_broker_ref_root waits on message_notify until the window
  deadline instead of re-polling every 3s, matching next_message_broker_ref
  and the azdo path. Heartbeats stay once per 50s window via the
  handlers' up-front touch_session.
- retire_node_requests no longer selects already-settled requests when
  retiring (settle is first-result-wins, so the second settle was pure
  waste: ~5 statements matching zero rows per completion).
- Regression tests: an idle poll leaves last_seen_at untouched, and a
  poll right after submit still claims.

* Sample the ready-queue depth from the 5s sampler instead of counting per operation

Every submit, claim, completion, cancel and promotion ran SELECT count(*)
over the jobs table to refresh a gauge whose only reader is the runner
pool supervisor. At target throughput that is ~140 full scans a second
for a number nobody needs exact.

- The 5s state sampler already computes the ready depth (status_inputs'
  grouped bucket count, published as jobs.ready). It now also stores it
  into the node-local queue_depth atomic the co-hosted pool scales off,
  and mirrors it into pool_status.
- Removed queue_depth from SubmitOutcome, ClaimedJob, CompleteOutcome,
  CancelOutcome, PromoteOutcome, EnvironmentApprovalOutcome and the azdo
  claim variant, plus JobSettled.queue_len (write-only).
- settle_job and the cancel paths use cheap EXISTS checks for the
  queue_nonempty wake signal instead of the count.
- Deleted the now-dead pg queue_depth()/queue_depth_on() and the
  uncalled pg queue_gauges helper; lite's queue_gauges no longer counts.
- The per-operation ready-front labels read stays: it is one indexed
  LIMIT 1 row, not a full scan.

* Wake the state sampler early on submit bursts (debounced)

The 5s sampler now feeds the pool's queue-depth gauge, but a burst could
sit up to 5s before the pool noticed - 10x the 500ms snapshot-fork
provision time. The sampler loop now also wakes on message_notify (which
the submit path already shouts into): Notify coalesces a flurry into one
wakeup, and a 1s floor caps a sustained storm at one extra grouped count
per second. Same epoll-style rhythm-plus-urgency pattern as the broker
long-poll.

* fix: isolate the sampler's wake channel; close long-poll lost-wake race

Review follow-ups on the poll-write refactor:

- Give the state sampler its own Notify (sampler_notify). wake_waiters
  fires per-job notify_one permits on message_notify; a sampler parked on
  the same channel could steal them and strand a runner until its window
  ended. Every producer now wakes both channels; the LISTEN relay too.
- Register the long-poll Notified before probing (enable()), in all three
  loops: broker ref, broker root, disttask. A notify_waiters landing
  between probe and registration stored no permit and was lost.
- Sampler debounce naps off the 1s floor instead of dropping the event,
  and MissedTickBehavior::Delay prevents a burst publish after a nap.
- Clamp waitSeconds to PRELOOP_MAX_POLL_WINDOW_SECS (default 60s) and
  floor PRELOOP_RUNNER_LIVENESS_TIMEOUT_SECS at 3x that window, so a
  client-chosen window can never park a runner past its own reaping.
- pg probe_poll: document that claim-enabling reads must never route to
  a replica (they can't see writer commits).
- queue_depth tests: drive a real sampler tick via test_sample_state_once
  (extracted from run_state_sampler) instead of asserting the removed
  per-operation write.

---------

Co-authored-by: Bill Njoroge <Bnjoroge1@users.noreply.github.com>
…se comment

- collect_job_secret_reads marks the job dynamic when a step has a
  non-Docker uses:. Composite inner steps evaluate against the job's
  secrets context, and action bodies cannot be inspected at submit time,
  so the job keeps the full scope rather than silently starving the
  composite of a secret it reads directly.
- docs/fidelity-gap.md: toJSON(secrets)/secrets.* are dynamic reads that
  keep the full scope, so they enumerate the same keys as GitHub. The
  actual divergence is direct literal references to unreferenced names
  evaluating to empty.
- runs.rs: the name filter uppercases as a deliberate superset; the
  evaluator itself does exact-case property lookup.
#351 replaced the scheme check with a configured-origin check so the static
PAT reaches a plain-http GitHub emulator (gh-simulate local mode) instead of
being dropped. The old assertion here — no Authorization header over plain
http — contradicted that deliberate behavior and failed the merged tree.
Pin the real contract instead: a configured origin receives the PAT
regardless of scheme; the unconfigured-origin half stays covered by
pat_targets_configured_github_regardless_of_scheme in actions.rs.
…uilds

cargo test shares one process environment across a whole test binary, so a
PRELOOP_GITHUB_TOKEN set by a neighbouring test (or injected into the job VM)
leaked into unrelated tests. Every submit then introspected that token
against the real GitHub API, and a fake one came back 401 -> 403 refusing to
embed an invalid PAT, failing whichever test happened to be submitting.
Test-support builds now treat the default GitHub API base as unverifiable and
never call out for it; a test that needs a verdict points
PRELOOP_GITHUB_API_URL at its own stub, which the guard leaves alone.
…sages (#364)

feat(secrets): inject only job-referenced stored secrets into job messages
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Too many files!

This PR contains 228 files, which is 128 over the limit of 100.

To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch.

Upgrade to a paid plan to raise the limit.

This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7ae52ba8-7398-41a5-bdc7-f3f9f032f1a0
📥 Commits

Reviewing files that changed from the base of the PR and between 7596af6 and 53f1f15.

⛔ Files ignored due to path filters (10)
  • docs/control-schema.dot is excluded by !**/*.dot
  • docs/control-schema.png is excluded by !**/*.png
  • docs/diagrams/01-system-overview.svg is excluded by !**/*.svg
  • docs/diagrams/02-control-plane.svg is excluded by !**/*.svg
  • docs/diagrams/03-runner-listener-worker.svg is excluded by !**/*.svg
  • docs/diagrams/04-parser-pipeline.svg is excluded by !**/*.svg
  • docs/diagrams/05-expression-engine.svg is excluded by !**/*.svg
  • docs/diagrams/06-protocol-layer.svg is excluded by !**/*.svg
  • docs/diagrams/07-orchestrator-backends.svg is excluded by !**/*.svg
  • docs/diagrams/08-job-lifecycle.svg is excluded by !**/*.svg
📒 Files selected for processing (228)
  • .github/workflows/ci.yml
  • .github/workflows/control-plane.yml
  • .github/workflows/release.yml
  • .gitignore
  • .runner-watch/state.json
  • CHANGELOG.md
  • benchmarks/control-load/LOAD-REPORT.md
  • benchmarks/control-load/run-round.sh
  • benchmarks/control-load/src/runner.rs
  • benchmarks/real-world/conformance-5repos.sh
  • crates/preloop-cli/src/debug_session.rs
  • crates/preloop-cli/src/github_setup.rs
  • crates/preloop-cli/src/main.rs
  • crates/preloop-cli/src/push.rs
  • crates/preloop-cli/src/update.rs
  • crates/preloop-conformance/src/main.rs
  • crates/preloop-gha-expressions/src/context.rs
  • crates/preloop-gha-expressions/src/evaluator.rs
  • crates/preloop-gha-expressions/src/lib.rs
  • crates/preloop-gha-expressions/src/lib_tests.rs
  • crates/preloop-gha-parser/src/job_builder.rs
  • crates/preloop-gha-parser/src/lib.rs
  • crates/preloop-gha-parser/src/lib_tests.rs
  • crates/preloop-gha-parser/src/models.rs
  • crates/preloop-gha-parser/src/secrets.rs
  • crates/preloop-gha-parser/tests/dynamic_matrix_repro.rs
  • crates/preloop-gha-protocol/src/azdo/azdo_tests.rs
  • crates/preloop-gha-protocol/src/azdo/context_data.rs
  • crates/preloop-gha-protocol/src/azdo/job.rs
  • crates/preloop-gha-protocol/src/debug_session.rs
  • crates/preloop-gha-protocol/src/lib.rs
  • crates/preloop-observability/src/status.rs
  • crates/preloop-orchestrator/src/lib.rs
  • crates/preloop-runner-client/src/main.rs
  • crates/preloop-runner-server/Cargo.toml
  • crates/preloop-runner-server/proptest-regressions/concurrency_properties.txt
  • crates/preloop-runner-server/src/actions.rs
  • crates/preloop-runner-server/src/artifact_twirp.rs
  • crates/preloop-runner-server/src/auth.rs
  • crates/preloop-runner-server/src/blob_store.rs
  • crates/preloop-runner-server/src/bootstrap.rs
  • crates/preloop-runner-server/src/broker.rs
  • crates/preloop-runner-server/src/cache_artifacts.rs
  • crates/preloop-runner-server/src/compat_ghes.rs
  • crates/preloop-runner-server/src/concurrency.rs
  • crates/preloop-runner-server/src/concurrency_http_properties.rs
  • crates/preloop-runner-server/src/concurrency_properties.rs
  • crates/preloop-runner-server/src/config.rs
  • crates/preloop-runner-server/src/control/backend.rs
  • crates/preloop-runner-server/src/control/lite/acquire.rs
  • crates/preloop-runner-server/src/control/lite/codec.rs
  • crates/preloop-runner-server/src/control/lite/concurrency.rs
  • crates/preloop-runner-server/src/control/lite/dispatch.rs
  • crates/preloop-runner-server/src/control/lite/expansion.rs
  • crates/preloop-runner-server/src/control/lite/fork_gate.rs
  • crates/preloop-runner-server/src/control/lite/impls.rs
  • crates/preloop-runner-server/src/control/lite/jobs.rs
  • crates/preloop-runner-server/src/control/lite/lifecycle.rs
  • crates/preloop-runner-server/src/control/lite/mod.rs
  • crates/preloop-runner-server/src/control/lite/poll.rs
  • crates/preloop-runner-server/src/control/lite/promote.rs
  • crates/preloop-runner-server/src/control/lite/queries.rs
  • crates/preloop-runner-server/src/control/lite/reaper.rs
  • crates/preloop-runner-server/src/control/lite/requests.rs
  • crates/preloop-runner-server/src/control/lite/runners.rs
  • crates/preloop-runner-server/src/control/lite/schema.sql
  • crates/preloop-runner-server/src/control/lite/settle.rs
  • crates/preloop-runner-server/src/control/lite/steps.rs
  • crates/preloop-runner-server/src/control/lite/submit.rs
  • crates/preloop-runner-server/src/control/lite/tests.rs
  • crates/preloop-runner-server/src/control/lite/testview.rs
  • crates/preloop-runner-server/src/control/lite/timelines.rs
  • crates/preloop-runner-server/src/control/lite/webhooks.rs
  • crates/preloop-runner-server/src/control/logic.rs
  • crates/preloop-runner-server/src/control/mod.rs
  • crates/preloop-runner-server/src/control/pg/codec.rs
  • crates/preloop-runner-server/src/control/pg/dispatch.rs
  • crates/preloop-runner-server/src/control/pg/graph.rs
  • crates/preloop-runner-server/src/control/pg/lifecycle.rs
  • crates/preloop-runner-server/src/control/pg/lookups.rs
  • crates/preloop-runner-server/src/control/pg/mod.rs
  • crates/preloop-runner-server/src/control/pg/outbox.rs
  • crates/preloop-runner-server/src/control/pg/reaper.rs
  • crates/preloop-runner-server/src/control/pg/runners.rs
  • crates/preloop-runner-server/src/control/pg/schema.sql
  • crates/preloop-runner-server/src/control/pg/tests.rs
  • crates/preloop-runner-server/src/control/pg/testview.rs
  • crates/preloop-runner-server/src/control/pg/timelines.rs
  • crates/preloop-runner-server/src/control/pg/trait_impl.rs
  • crates/preloop-runner-server/src/control/pg/webhooks.rs
  • crates/preloop-runner-server/src/control/tests.rs
  • crates/preloop-runner-server/src/control/testview.rs
  • crates/preloop-runner-server/src/control/txn_stats.rs
  • crates/preloop-runner-server/src/control/types.rs
  • crates/preloop-runner-server/src/control/wake.rs
  • crates/preloop-runner-server/src/credential_store.rs
  • crates/preloop-runner-server/src/debug.rs
  • crates/preloop-runner-server/src/debug_sessions.rs
  • crates/preloop-runner-server/src/dispatch.rs
  • crates/preloop-runner-server/src/dispatch_auth.rs
  • crates/preloop-runner-server/src/dispatch_tests.rs
  • crates/preloop-runner-server/src/distributed_task.rs
  • crates/preloop-runner-server/src/errors.rs
  • crates/preloop-runner-server/src/event_feed.rs
  • crates/preloop-runner-server/src/events/trust_tier.rs
  • crates/preloop-runner-server/src/fork_policy.rs
  • crates/preloop-runner-server/src/github.rs
  • crates/preloop-runner-server/src/github_app.rs
  • crates/preloop-runner-server/src/github_pr.rs
  • crates/preloop-runner-server/src/github_push.rs
  • crates/preloop-runner-server/src/lib.rs
  • crates/preloop-runner-server/src/live_log_segments.rs
  • crates/preloop-runner-server/src/live_logs.rs
  • crates/preloop-runner-server/src/memory_caps.rs
  • crates/preloop-runner-server/src/message_template.rs
  • crates/preloop-runner-server/src/models.rs
  • crates/preloop-runner-server/src/oauth.rs
  • crates/preloop-runner-server/src/oidc_handlers.rs
  • crates/preloop-runner-server/src/openapi.rs
  • crates/preloop-runner-server/src/remote_workflows.rs
  • crates/preloop-runner-server/src/results_twirp.rs
  • crates/preloop-runner-server/src/retention.rs
  • crates/preloop-runner-server/src/routes.rs
  • crates/preloop-runner-server/src/runner_lifecycle.rs
  • crates/preloop-runner-server/src/runs.rs
  • crates/preloop-runner-server/src/runtime_scheduling.rs
  • crates/preloop-runner-server/src/scheduler.rs
  • crates/preloop-runner-server/src/secret_provider.rs
  • crates/preloop-runner-server/src/secrets_api.rs
  • crates/preloop-runner-server/src/shared_http.rs
  • crates/preloop-runner-server/src/snapshots.rs
  • crates/preloop-runner-server/src/state.rs
  • crates/preloop-runner-server/src/store.rs
  • crates/preloop-runner-server/src/store_pg.rs
  • crates/preloop-runner-server/src/test_pg.rs
  • crates/preloop-runner-server/src/timeline_logs.rs
  • crates/preloop-runner-server/src/webhook_api.rs
  • crates/preloop-runner-server/src/webhook_watchdog.rs
  • crates/preloop-runner-server/tests/broker.rs
  • crates/preloop-runner-server/tests/common/mod.rs
  • crates/preloop-runner-server/tests/concurrency.rs
  • crates/preloop-runner-server/tests/logs.rs
  • crates/preloop-runner-server/tests/recovery.rs
  • crates/preloop-runner-server/tests/registration.rs
  • crates/preloop-runner-server/tests/runs_api.rs
  • crates/preloop-runner-server/tests/security.rs
  • crates/preloop-runner-server/tests/webhooks.rs
  • crates/preloop-runner/src/cli.rs
  • crates/preloop-runner/src/client/actions_download.rs
  • crates/preloop-runner/src/client/http.rs
  • crates/preloop-runner/src/client/results.rs
  • crates/preloop-runner/src/configure.rs
  • crates/preloop-runner/src/control_bridge.rs
  • crates/preloop-runner/src/listener/broker_listener.rs
  • crates/preloop-runner/src/listener/job_dispatcher.rs
  • crates/preloop-runner/src/listener/message_listener.rs
  • crates/preloop-runner/src/listener/oauth.rs
  • crates/preloop-runner/src/main.rs
  • crates/preloop-runner/src/process.rs
  • crates/preloop-runner/src/settings.rs
  • crates/preloop-runner/src/worker/actions/manager.rs
  • crates/preloop-runner/src/worker/commands.rs
  • crates/preloop-runner/src/worker/completion.rs
  • crates/preloop-runner/src/worker/container_ops.rs
  • crates/preloop-runner/src/worker/contexts.rs
  • crates/preloop-runner/src/worker/contexts_tests.rs
  • crates/preloop-runner/src/worker/debug_pause.rs
  • crates/preloop-runner/src/worker/debug_pause_tests.rs
  • crates/preloop-runner/src/worker/file_commands.rs
  • crates/preloop-runner/src/worker/handlers/action.rs
  • crates/preloop-runner/src/worker/handlers/composite.rs
  • crates/preloop-runner/src/worker/handlers/container.rs
  • crates/preloop-runner/src/worker/handlers/factory.rs
  • crates/preloop-runner/src/worker/handlers/node.rs
  • crates/preloop-runner/src/worker/handlers/script.rs
  • crates/preloop-runner/src/worker/job_extension.rs
  • crates/preloop-runner/src/worker/job_extension_tests.rs
  • crates/preloop-runner/src/worker/job_runner.rs
  • crates/preloop-runner/src/worker/job_runner_tests.rs
  • crates/preloop-runner/src/worker/matchers.rs
  • crates/preloop-runner/src/worker/official_oracles.rs
  • crates/preloop-runner/src/worker/reporting.rs
  • crates/preloop-runner/src/worker/server_queue.rs
  • crates/preloop-runner/src/worker/steps_runner.rs
  • crates/preloop-runner/src/worker/steps_runner_tests.rs
  • crates/preloop-runner/src/worker/template.rs
  • crates/preloop-runner/src/worker/workspace_diff.rs
  • crates/preloop-runner/tests/bridge_limits.rs
  • crates/preloop-runner/tests/oom_caps.rs
  • crates/runner-watch/src/compare.rs
  • crates/runner-watch/src/main.rs
  • docs/architecture.md
  • docs/conformance.md
  • docs/control-schema-chartdb.json
  • docs/control-schema.sql
  • docs/debug-sessions.md
  • docs/diagrams/01-system-overview.html
  • docs/diagrams/01-system-overview.json
  • docs/diagrams/02-control-plane.html
  • docs/diagrams/02-control-plane.json
  • docs/diagrams/03-runner-listener-worker.html
  • docs/diagrams/03-runner-listener-worker.json
  • docs/diagrams/04-parser-pipeline.html
  • docs/diagrams/05-expression-engine.html
  • docs/diagrams/06-protocol-layer.html
  • docs/diagrams/06-protocol-layer.json
  • docs/diagrams/07-orchestrator-backends.html
  • docs/diagrams/07-orchestrator-backends.json
  • docs/diagrams/08-job-lifecycle.html
  • docs/diagrams/08-job-lifecycle.json
  • docs/diagrams/index.html
  • docs/diagrams/render.py
  • docs/fidelity-gap.md
  • docs/github-app-api-compat.md
  • docs/self-hosting.md
  • docs/setup.md
  • experiments/mitm/FORMAT.md
  • experiments/mitm/addons/capture.py
  • experiments/mitm/addons/hosts.py
  • experiments/mitm/bin/_compare.py
  • experiments/mitm/bin/extract-hosts.py
  • experiments/mitm/bin/pull-app-deliveries.py
  • experiments/mitm/bin/reconstruct_scenario.py
  • experiments/mitm/scenarios/201-expression-edge-cases/201-expression-edge-cases.yml
  • experiments/mitm/scenarios/207-masking-secrets-vars/207-masking-secrets-vars.yml
  • experiments/mitm/scenarios/211-job-container-volumes/211-job-container-volumes.yml
  • experiments/mitm/scenarios/216-summaries-env-cascade/216-summaries-env-cascade.yml
  • experiments/mitm/tests/test_capture_kit.py

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Job messages now carry only referenced secret names, so the fixture's step
must read ${{ secrets.NPM_TOKEN }} for the name to reach preloopSecretSpec —
and for the acquire fill to surface its value. The assertion's intent is
unchanged: the run's own secret is named, the debug worker token never is.

@superagent-security superagent-security Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superagent found 1 security concern(s).

let mut request = crate::shared_http::CLIENT.get(&url);
if let Some(pat) = state.static_github_pat()
&& url.starts_with("https://")
&& url_targets_configured_github(&url, &state.github_urls)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Static PATs are now sent over plaintext HTTP to configured GitHub endpoints.

The new host/port check ignores scheme, so a configured HTTP API URL receives the static PAT as a Bearer token.

Require HTTPS for PAT-bearing requests; allow HTTP emulators only without the PAT (or with a loopback-only exception).

AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="crates/preloop-runner-server/src/actions.rs">
<violation number="1" location="crates/preloop-runner-server/src/actions.rs:225">
<priority>P1</priority>
<title>Static PATs are now sent over plaintext HTTP to configured GitHub endpoints.</title>
<evidence>The added `url_targets_configured_github` check (lines 178–194) compares only the target host and effective port with configured GitHub endpoints; it deliberately does not require HTTPS. `resolve_ref_to_sha` now attaches `state.static_github_pat()` as a Bearer token when that check passes (lines 224–227), and `download_action_tarball` does the same for the tarball request (lines 390–393). Since `PRELOOP_GITHUB_API_URL` and the corresponding config value are used as supplied, configuring a non-loopback `http://` forge causes the static PAT to cross the network in cleartext. A network-path observer can capture the credential and use its verified OAuth scopes. A workflow's remote action references reach these requests through the runner's `runnerresolve/actions` batch call (`crates/preloop-runner/src/client/actions_download.rs:85–123`).</evidence>
<recommendation>Require HTTPS before attaching a static PAT, preserving the previous scheme guard. If local HTTP emulators must remain supported, allow unauthenticated HTTP requests or add an explicit loopback-only exception that never transmits the PAT; add tests for both lookup and tarball requests.</recommendation>
</violation>
</file>

Queue depth is refreshed by the 5s sampler, not per submit/claim, so the
webhook test's assertion read a stale 0 — the run itself was queued (the
working-set view shows it). Run one sampler tick inline first, the same
convention logs.rs uses; the test harness spawns no sampler task.
@Bnjoroge1

Copy link
Copy Markdown
Collaborator Author

Superseded by the rebased PR: #347 merged as a rebase, so diffing from this branch showed ~325 already-landed commits. The replacement contains only the post-#347 work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant