[WIP][CRCR] Initial implementation of L2#7967
[WIP][CRCR] Initial implementation of L2#7967KarhouTam wants to merge 5 commits intopytorch:mainfrom
Conversation
Co-authored-by: can-gaa-hou <jiahaochen535@gmail.com> Co-authored-by: fffrog <ljw1101.vip@gmail.com>
|
Hi @KarhouTam! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
@KarhouTam is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
KarhouTam
changed the title
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
meta-cla
Bot
added
the
CLA Signed
|
|
||
| #### Known limitations of this model | ||
|
|
||
| A compromised or malicious maintainer of an allowlisted repo can: |
There was a problem hiding this comment.
@ZainRizvi raised this in the RFC Document: What's the worst that a malicious user could do with these known limitations?
There was a problem hiding this comment.
Maybe a higher CI passing rate could benefit them to upgrade from L2 to L3 I guess. I believe most people won't cheat on that, but we must consider such a situation in case it happens.
| # a tampered value just misses the timing cache, which only hurts the | ||
| # attacker's own HUD row. | ||
| ci_metrics: dict = {"queue_time": None, "execution_time": None} | ||
| if status == "in_progress": |
There was a problem hiding this comment.
Consider adding a state machine to enforce valid status transitions before processing. Right now any allowlisted repo can send in_progress or completed in any order with no checks. A simple Redis-backed state per delivery_id + verified_repo would prevent rollbacks and replays:
in_progress: accepted only if no record exists yet
completed: accepted if current status is in_progress OR no record exists (handles missed in_progress callback)
in_progress after completed: rejected (no rollback)
Duplicate completed: rejected
This combined with per-repo rate limiting covers replay and flooding concerns without needing complex tamper-proof logic on every field. The OIDC-verified downstream_repo is the trust anchor — trusting the data from a verified source is reasonable since the downstream is invested in accuracy.
The in_progress skip for completed is important — the in_progress callback is best-effort and can be lost (network issues, Lambda timeout). Rejecting completed in that case would mean silently losing the actual test results, which is worse than missing the queue_time metric.
There was a problem hiding this comment.
The state machine approach is solid, but we must consider what happens if the downstream workflow fails due to internal issues and requires a rerun. In that scenario, the relay would receive multiple in_progress payloads, making it indistinguishable from a replay attack—the only difference being the intent. Given that network instability, hardware failures, and software bugs are common, such downstream failures are highly probable and must be accounted for.
There was a problem hiding this comment.
@subinz1 @KarhouTam @can-gaa-hou
I agree to add state machine back(We added it in POC state, but removed it due to a little more complexity), but this logic involves several details.
Background
Starting from L3, we need to support the rerun functionality on both the PyTorch side and the downstream side. Essentially, the handling logic for both rerun scenarios is the same. Considering network fluctuations, user behavior, and reruns, I suggest simplifying the state machine:
Metrics
dispatch_time: This is straightforward. It is recorded in Redis as soon as webhook_lambda forwards the event to the downstream repo.started_at: Recorded once and only once. Regardless of whether the request is an initial run or a rerun, we only keep the timestamp of the first occurrence in Redis.completed_at: Same as above, recorded once and only once. Subsequent rerun updates will not overwrite this time.
Limitations and Justification:
callback_handlerensures that these timestamp fields are not empty when delivering data to HUD.- While the final calculated execution and queue times might be "magnified" (less accurate) during reruns, I believe this error is reasonable and acceptable at this stage since it is primarily driven by downstream behavior.
Test Result Uploads and State Transitions
To reach a consensus, I suggest defining the following three STATUS values in Redis:
Started: dispatch has been successfully sent.In_progress: "In-progress" feedback has been received.Completed: Final test results have been received.
(Note: There is also an implicit None state, meaning no record exists in Redis for the given delivery_id.)
When callback_lambda receives a callback signal, the logic is as follows:
A. Upon receiving an in_progress signal:
None: Reject. This indicates the task was not triggered by a valid webhook. Whether it is an initial run or a rerun, a record should exist in Redis (we set a 3-day expiration to align with the maximum job execution time).Started: Accept and call the HUD API to notify that the task has officially started.In_progress: Accept, but do not call the HUD API (to ensure the workflow continues while avoiding redundant calls).Completed: Accept, update the HUD Status toIn progess.
B. Upon receiving a complete signal:
None: Reject. (Same reason as above).Started: Reject. In this abnormal case (skipping the in_progress stage), the user can use a rerun in the downstream repo to re-upload, so no test results will be lost.In_progress: Accept and call the HUD API (initial creation or updating).Completed: Reject
This is just a preliminary thought, and I'd love to hear your feedback.
There was a problem hiding this comment.
but we must consider what happens if the downstream workflow fails due to internal issues and requires a rerun. In that scenario, the relay would receive multiple in_progress payloads, making it indistinguishable from a replay attack—the only difference being the intent
The github actions api's attempt field helps with this:
- When a workflow is executed, it creates a new Run with
attempt=1 - If jobs fail and get retried, a new event is emitted with the same run number but
attemptis incremented. - job_id remains unique per job execution, so it should never be affected by reruns (reruns create a new job id)
So our state machine should include tracking: run_id, run_attempt, and job_id.
There was a problem hiding this comment.
In_progress: Accept, but do not call the HUD API (to ensure the workflow continues while avoiding redundant calls).
I also want to call out is that HUD should prob get an entry back per job as well as per callback. The way I'm thinking about it, the OOT repo could want to trigger multiple jobs for a single commit, and each one should be able to report back it's own status. This allows the OOT to say "these four jobs passed but this one failed"
One callback could potentially call 5 jobs under the hood, and we want to ensure all 5 get status updates in HUD
| "in_progress". | ||
| required: false | ||
| default: '' | ||
| test-results: |
There was a problem hiding this comment.
Hi @KarhouTam,
As per @ZainRizvi' comments here: https://docs.google.com/document/d/1w10Agz_YkSA1IDurobWjuNoYo4u9aykprjuymDFE6MU/edit?disco=AAAB3T8nxDE, it is tricky to enforce a consistent test result json schema, and also including all the failed test info may become very large. So it is better to call-out or enforce a standard test result json which the downstream sends to result_handler lambda.
A standard test result json is also needed for HUD integration.
WDYT
There was a problem hiding this comment.
Agree that regulate the test-results is necessary. I think we should settle down the final HUD RFC first to have a clear direction of code implementation.
There was a problem hiding this comment.
Sure.
We can add annotations to this action to clarify for downstream developers what they should and should not deliver. We could also evaluate the "body" content received from the downstream repo. However, the prerequisite for this is having a clear definition of the "body" in the HUD RFC.
Would you mind proposing the RFC first? I believe this will help us reach a final consensus, which can then guide us in developing and optimizing the code.
There was a problem hiding this comment.
Just to make sure we're on the same page here, I think we're landing at:
- This
test-resultsobject only contains a summary of the tests (e.g. X tests passed, Y failed, Z skipped. Anything else needed?). Importantly, it does not enumerate all passing/failing tests. - Full test result info gets uploaded as an artifact to some url
Also: We should add a version number to this schema in case we need to update it in the future without breaking backwards compatibility.
| |---|---|---| | ||
| | `2xx` | record delivered | green | | ||
| | `4xx` (schema reject) | propagate same status | **red** — author must fix payload | | ||
| | `5xx` / network error | log + swallow | green — HUD outage is not the caller's fault | |
There was a problem hiding this comment.
log + swallow
minor discrepancy here: We should log and return the error to the caller, letting them know that there was some internal error and the data was not saved as they expect (that's us being honest). The code returns the error instead of swallowing it
But our documentation and return message should let the caller should be told that these internal errors should be considered ignorable by them and not fail their CI.
Why put this in the return message? It'll probably get read by an LLM. We're helping it code :)
ZainRizvi
left a comment
ZainRizvi
left a comment
There was a problem hiding this comment.
Some earlier comments plus a couple new ones from cross-referencing with the RFC.
| last_exception.code, | ||
| last_exception.reason, | ||
| ) | ||
| raise HTTPException(last_exception.code, last_exception.reason) from last_exception |
There was a problem hiding this comment.
Prob shouldn't log the internal code and exception reason here. If it's an internal failure (not the caller's fault) it's valid to just return a 500 and say something like this to guide the LLM reading the failure:
"An internal failure occured. Your update was note saved, but the CI run is still valid. You can attempt progressive retries after X seconds or ignore this failure."
| in_progress_at, completed_at, "execution_time" | ||
| ) | ||
|
|
||
| trusted = {"ci_metrics": ci_metrics, "verified_repo": verified_repo} |
There was a problem hiding this comment.
Let's also include the downstream repo level here, so that this lambda is the only determiner of authoritative repo level information at this point in time.
If we make HUD recompute this information when we make tiering information dynamic, that logic will have to be kept in sync with HUD (and we get exposed to other timing/consistency issues).
| "in_progress". | ||
| required: false | ||
| default: '' | ||
| test-results: |
There was a problem hiding this comment.
Just to make sure we're on the same page here, I think we're landing at:
- This
test-resultsobject only contains a summary of the tests (e.g. X tests passed, Y failed, Z skipped. Anything else needed?). Importantly, it does not enumerate all passing/failing tests. - Full test result info gets uploaded as an artifact to some url
Also: We should add a version number to this schema in case we need to update it in the future without breaking backwards compatibility.
| supply the relay URL, the status/conclusion, and optional structured test | ||
| results. | ||
|
|
||
| inputs: |
There was a problem hiding this comment.
This will need an artifact url too, right?
| description: > | ||
| Base URL of the result callback server. | ||
| required: false | ||
| default: https://zciswjsrynb6ksccc2nb3mckpa0ldzou.lambda-url.us-east-1.on.aws/github/result |
There was a problem hiding this comment.
minor: I know this is for testing, but might be easier to avoid an accidental merge by moving this to the caller sooner rather than later :)
ZainRizvi
left a comment
ZainRizvi
left a comment
There was a problem hiding this comment.
A couple more things from cross-referencing with the RFC.
| field; HUD should prefer it over anything self-reported in `body`. | ||
| - **Schema / business validation (HUD's job)**: the callback body is passed | ||
| through to HUD verbatim as a top-level `body` field. Relay does **not** | ||
| validate fields inside `body.workflow` or `body.payload` — HUD owns the |
There was a problem hiding this comment.
validate fields inside
body.workfloworbody.payload— HUD owns the schema since it owns persistence.
This contradicts the direction we landed on in the RFC review (rfcs#96) — validation should live in the relay, not HUD. The relay is already the gatekeeper for OIDC, allowlist, and rate limiting, so schema validation belongs here too. HUD should just authenticate the relay and write what it's told.
The code actually does the right thing already (validates delivery_id and workflow.status here in the relay). Let's update this section to match what the code does instead of claiming the opposite.
|
|
||
| _ALLOWLIST_CACHE_KEY = "crcr:allowlist_yaml" | ||
| _TIMING_PREFIX = "crcr:timing:" | ||
| _RATE_LIMIT_PREFIX = "crcr:ratelimit:" |
There was a problem hiding this comment.
Thoughts on using oot: as the prefix for all the Redis keys? Right now it's crcr: for timing/allowlist and crcr:ratelimit: here. The RFC and doc use oot:rate:. Either prefix is fine, but let's pick one — a split between crcr: and oot: for different keys would be the worst outcome. I'd lean oot: since that's what the RFC uses and it's more descriptive for anyone poking around in Redis.
|
Hey, @ZainRizvi . Thanks for your valuable comments and suggestions. I am on Labor Day vacation now and will be back on May 6th. I will address these when I come back! Once again, thank you! |
| "Authorization": config.hud_bot_key, | ||
| }, | ||
| method="POST", | ||
| ) |
There was a problem hiding this comment.
The RFC (rfcs#96) just changed this to a dedicated X-OOT-Relay-Token header — the whole point was to avoid reusing a shared credential on a write path. This should send X-OOT-Relay-Token instead of Authorization so HUD can scope the token to just the /api/oot/results endpoint.
| "name": os.environ["WORKFLOW_NAME"], | ||
| "url": os.environ["WORKFLOW_URL"], | ||
| } | ||
|
|
There was a problem hiding this comment.
The RFC's dynamoKey is {repo}/{delivery_id}/{workflow_name}/{job_name}/{run_attempt}, which means HUD expects job_name, run_attempt, started_at, and completed_at in the workflow dict. Without them, every job in a multi-job workflow will collide on the same key (job_name defaults to "default", run_attempt to 1), and HUD has no wall-clock timestamps so duration_seconds is always 0.
This needs to include:
"job_name": os.environ["JOB_NAME"],
"run_attempt": int(os.environ["RUN_ATTEMPT"]),
"started_at": datetime.utcnow().isoformat() + "Z", # only on in_progress
"completed_at": datetime.utcnow().isoformat() + "Z", # only on completedwith corresponding env vars:
JOB_NAME: ${{ github.job }}
RUN_ATTEMPT: ${{ github.run_attempt }}| in_progress_at, completed_at, "execution_time" | ||
| ) | ||
|
|
||
| trusted = {"ci_metrics": ci_metrics, "verified_repo": verified_repo} |
There was a problem hiding this comment.
Following up on my earlier comment about including downstream_repo_level — the allowlist is already loaded a few lines above and verified_repo is checked against l2_repos. The level should be set here in trusted so HUD doesn't need to recompute it:
level = allowlist.get_level(verified_repo) # or however the allowlist exposes this
trusted = {"ci_metrics": ci_metrics, "verified_repo": verified_repo, "downstream_repo_level": level.value}The RFC schema includes this field and HUD will need it for the per-backend dashboard.
ZainRizvi
left a comment
ZainRizvi
left a comment
There was a problem hiding this comment.
As a general prompting tip, claude does really well if you point it at both the RFC PR and the google doc we worked and and ask it to look for inconsistencies between it (and explicitly ask it to check the comments as well).
That's a good prompt for both the coding agent to start with and also for the reviewing agent to verify against.
6 participants
Author
Summary
Higher-level behaviors for
L3andL4are intentionally left for follow-up work.Architecture
The relay is split into two AWS Lambda functions:
webhooklambda function (Updated)opened/reopened/synchronized/closedactions, forwards repository_dispatch events to downstream reposcallbacklambda function (Added)queue timeandexecute timefor evolution toL3repoChanges
..github/ ├── workflows/ │ └── _lambda-do-release-runners.yml # Updates the Lambda release workflow to include cross-repo-ci-relay packaging/release │ └── actions/ └── cross-repo-ci-relay-callback/ └── action.yml # Composite action used by downstream workflows to report status back to the relay/result endpoint aws/lambda/cross_repo_ci_relay/ ├── tests/ # Unit tests for allowlist/config/webhook/result/redis behavior ├── README.md # Project overview, local development, callback flow, and result-side validation steps ├── Makefile # Top-level local developer entrypoint for test / deploy / clean ├── local_server.py # FastAPI wrapper for local end-to-end testing of both webhook and result endpoints ├── requirements.txt # Python dependencies required by the relay Lambdas │ ├── utils/ │ ├── allowlist.py # Loads, parses, and queries the downstream allowlist by rollout level │ ├── config.py # Shared runtime config loading and cached get_config() helper │ ├── gh_helper.py # GitHub App, repository_dispatch, and GitHub file access helpers │ ├── hud.py # HUD write helpers for downstream result reporting │ ├── jwt_helper.py # Helpers for minting/verifying relay callback tokens │ ├── redis_helper.py # Redis helpers for allowlist cache, OOT state, and timing data │ └── misc.py # Shared TypedDict definitions and HTTPException │ ├── webhook/ │ ├── Makefile # Build/package/deploy commands for the webhook Lambda │ ├── lambda_function.py # Webhook Lambda entrypoint: verifies GitHub webhook requests and routes events │ └── event_handler.py # Handles PR/push events, resolves allowlist targets, and dispatches to downstream repos │ └── callback/ ├── Makefile # Build/package/deploy commands for the result Lambda ├── lambda_function.py # Result Lambda entrypoint: verifies callback token and GitHub OIDC token └── callback_handler.py # Validates callback payloads, checks L2+ eligibility, stores state, and writes to HUDUsage
See README.md for more details.
Verification
We performed the following scenario verification on our AWS Lambda instance:
Terraform configuration
Unit Tests
Security
Callback payload carries full upstream webhook data back to HUD —
action.ymlbuilds the callback body by mutatinggithub.event.client_payload(which contains the entire original webhook payload: PR metadata, commits, author info) and addingstatus/conclusion/workflow_name/workflow_urlon top. This full blob is forwarded verbatim byhud.pyto HUD with no relay-side filtering. HUD receives both relay-trustedverified_repoand an unvalidated body — if HUD trusts self-reported fields inside the body oververified_repo, a manipulated dispatch payload could tamper with HUD records.Lambda callback URL is public and hardcoded — The endpoint is hardcoded in `action.yml and exposed in a public action, making it trivially discoverable. OIDC verification blocks unauthorized HUD writes, but the endpoint has no rate limiting; request flooding can cause Lambda concurrency exhaustion or Redis connection saturation.
Only OIDC is used for verification — The callback lambda relies solely on GitHub OIDC token verification for authentication, without additional application-level secrets or signatures. If an attacker compromises a downstream repo's GitHub Actions permissions, they could forge authenticated requests to the callback endpoint. Besides, OIDC has its own limitations (e.g., token expiration, potential misconfigurations) that could lead to unauthorized access if not carefully managed.
HUD Interaction
Design Principle: Transparent Relay & Decoupling
The Relay Server acts as a lightweight data passthrough layer. It does not define or parse specific CI data formats; instead, it offloads data interpretation and validation to the HUD. This ensures complete decoupling between the relay infrastructure and business-specific data.
Security & Risk Mitigation
The relay uses OIDC authentication to guarantee the authenticity of the data source (Verified Repo). Its core responsibility is to ensure the data originates from the claimed repository, while security filtering and content compliance are enforced at the HUD level.