feat(medic): CI Medic mode for post-CI triage, retry, and auto-fix - #108
Conversation
Adds a workflow_run-triggered mode that runs after CI completes on a pull request. It waits for every check on the head commit to finish, diagnoses the failed jobs from their logs, classifies each failure, reruns jobs that look flaky or infrastructure related, and either commits a focused fix to the pull request branch or posts inline suggestions when auto_fix is off. Three budgets bound the cost, each with a distinct scope: max_retries caps reruns of one job for one commit, max_fix_attempts caps consecutive fix commits, and max_runs_per_pr caps invocations over the pull request lifetime and is enforced at the gate before Droid is invoked. Configuration comes from action inputs merged over an optional .github/droid-ci.yml read from the base branch, so a pull request cannot reconfigure the bot. Repository config is untrusted, so every field is coerced back to its declared type; a malformed value degrades to the default instead of leaving a nested object undefined for the gate checks. The drop-in template checks out the head branch so fixes have a real branch to commit to, and falls back to a detached checkout for fork branches that do not exist in the base repository. Also adds scripts/ci-medic-sandbox.ts, which provisions a sandbox repository and a fleet of pull requests covering each code path. workflow_run cannot be simulated locally because the triggering definition is read from the default branch and reruns go through the Actions API. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
GitHub does not start a new workflow run for a push made with the workflow's own GITHUB_TOKEN, which it blocks to prevent recursion. CI Medic pushed its auto-fix commits with that token, so a fix landed under the stale failing checks that triggered it and was never verified. Drops the github_token input from the template and adds id-token: write, which routes commits and comments through the Factory Droid App token. Reading failed job logs and rerunning jobs already used DEFAULT_WORKFLOW_TOKEN rather than the app token, so actions: write is still required and that path is unaffected. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
CI Medic allows github_inline_comment___create_inline_comment, but the server backing it was gated on isEntityContext. A workflow_run payload is not an entity context, so the server was never installed and the CLI rejected every medic run up front with "Unknown tool identifier(s)". That tool is how the auto_fix: false path delivers suggestions, so it has to install rather than be dropped from the list. Resolves the pull request number once, preferring the entity context and falling back to MEDIC_PR_NUMBER, and uses it for both the inline comment and CI servers instead of repeating the fallback per server. Adds a test asserting that every namespaced tool CI Medic allows has a server installed for a workflow_run context, which is the invariant that broke. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… per run The lifetime budget counted pull request comments containing the ci-medic marker, and each run created a new marker comment. Droid then rewrote that comment's body through update_droid_comment, which dropped the marker, so every successful run destroyed its own record. Observed on a sandbox pull request: three medic runs, one surviving marker. max_runs_per_pr would only ever have counted runs that crashed before Droid started, making the primary cost control close to inert. The count now lives inside the marker on a single tracking comment that each run reuses, and the comment server carries an existing marker forward when Droid replaces the body. Reusing one comment also stops the medic posting a fresh top-level comment on every run. Also tells Droid to read existing review comments before posting, since reruns were duplicating the same inline suggestion on the same line. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The scenario passed an unknown flag to bun test to force a failure whose only repair would live in a protected path. Bun ignores unknown flags, so the job passed and the scenario silently verified nothing. Misspells the script name instead, which fails for real. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
Droid finished @factory-nizar's task —— View job CI Medic is a solid addition, but there are a few high-impact correctness and security gaps: the config parser currently mis-parses common YAML layouts, numeric inputs cannot be set to zero, and the workflow_run path needs stronger trust boundaries (both for config ref selection and for running at all on untrusted PRs). |
EvidenceCI Medic was validated against a real repository, Factory-AI/ci-medic-sandbox, not mocks. 12 of 12 scenarios pass, 636 tests pass, and this branch adds CI to Two things are worth a reviewer's attention: the fix scope is enforced by withholding tools rather than by asking, and two behaviors remain unproven (listed at the bottom).
Every run charged exactly one budget unit and left exactly one tracking comment. The fix scope is enforced by withholding tools
Job names are matched by category, so Both directions proven live:
One commit is charged once, even when several workflows failEvery watched workflow that fails raises its own event, and the first run already waits for all checks on the commit, so the marker records the commit it analyzed and later events for that commit are skipped. Rather than wait for a race to demonstrate it, the CI fixture is dispatchable, so a duplicate attempt-1 run can be raised on an unchanged commit deliberately. On #38: The skip line is in that run's log. A failed rerun carries a higher attempt and is still analyzed, because that is a new outcome rather than a duplicate. Protected paths: two links proven, one not
Not covered
|
| const match = /^\s*([A-Za-z_]+):\s*(.+)$/.exec(line); | ||
| if (!match) continue; | ||
| const key = match[1]!; | ||
| const value = match[2]!; | ||
| const parsed = value.replace(/^['"]|['"]$/g, ""); | ||
| const target = section | ||
| ? (result[section] as Record<string, unknown>) | ||
| : result; | ||
| target[key] = parseScalar(parsed); |
There was a problem hiding this comment.
[P1] Fix YAML parser dropping top-level keys after sections
parseConfig keeps the last section active until another section header is seen, so a later top-level scalar like max_runs_per_pr: 5 gets written under the previous section object (often skip) and is then ignored by mergeConfig, which makes common configs silently not apply (including the pattern of putting max_runs_per_pr at the end of the file).
| const match = /^\s*([A-Za-z_]+):\s*(.+)$/.exec(line); | |
| if (!match) continue; | |
| const key = match[1]!; | |
| const value = match[2]!; | |
| const parsed = value.replace(/^['"]|['"]$/g, ""); | |
| const target = section | |
| ? (result[section] as Record<string, unknown>) | |
| : result; | |
| target[key] = parseScalar(parsed); | |
| const match = /^\s*([A-Za-z_]+):\s*(.+)$/.exec(line); | |
| if (!match) continue; | |
| const key = match[1]!; | |
| const value = match[2]!; | |
| const parsed = value.replace(/^['\"]|['\"]$/g, ""); | |
| const isNested = /^\s+/.test(line); | |
| const target = | |
| section && isNested | |
| ? (result[section] as Record<string, unknown>) | |
| : result; | |
| if (!isNested) section = ""; | |
| target[key] = parseScalar(parsed); |
| maxRetries: Math.max( | ||
| 0, | ||
| parseInt(process.env.MAX_RETRIES ?? "1", 10) || 1, | ||
| ), | ||
| maxFixAttempts: Math.max( | ||
| 0, | ||
| parseInt(process.env.MAX_FIX_ATTEMPTS ?? "2", 10) || 2, | ||
| ), | ||
| maxRunsPerPr: Math.max( | ||
| 1, | ||
| parseInt(process.env.MAX_RUNS_PER_PR ?? "10", 10) || 10, | ||
| ), |
There was a problem hiding this comment.
[P1] Allow numeric inputs to be set to "0"
The parseInt(...) || <default> pattern means inputs like MAX_RETRIES=0 (and similarly MAX_FIX_ATTEMPTS=0) are treated as “unset” and replaced with the default, so callers cannot express zero even though the surrounding Math.max(...) suggests 0 is a valid value.
| maxRetries: Math.max( | |
| 0, | |
| parseInt(process.env.MAX_RETRIES ?? "1", 10) || 1, | |
| ), | |
| maxFixAttempts: Math.max( | |
| 0, | |
| parseInt(process.env.MAX_FIX_ATTEMPTS ?? "2", 10) || 2, | |
| ), | |
| maxRunsPerPr: Math.max( | |
| 1, | |
| parseInt(process.env.MAX_RUNS_PER_PR ?? "10", 10) || 10, | |
| ), | |
| maxRetries: (() => { | |
| const parsed = parseInt(process.env.MAX_RETRIES ?? "1", 10); | |
| return Math.max(0, Number.isNaN(parsed) ? 1 : parsed); | |
| })(), | |
| maxFixAttempts: (() => { | |
| const parsed = parseInt(process.env.MAX_FIX_ATTEMPTS ?? "2", 10); | |
| return Math.max(0, Number.isNaN(parsed) ? 2 : parsed); | |
| })(), | |
| maxRunsPerPr: (() => { | |
| const parsed = parseInt(process.env.MAX_RUNS_PER_PR ?? "10", 10); | |
| return Math.max(1, Number.isNaN(parsed) ? 10 : parsed); | |
| })(), |
| ...((context.inputs.retryMode && context.inputs.retryMode !== "smart") || | ||
| (context.inputs.maxRetries && context.inputs.maxRetries !== 1) | ||
| ? { | ||
| retry: { | ||
| mode: context.inputs.retryMode ?? "smart", | ||
| max_per_job: context.inputs.maxRetries ?? 1, | ||
| eligible: [], | ||
| exclude: [], | ||
| }, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
[P1] Treat maxRetries=0 as an explicit override
The retry override gate uses (context.inputs.maxRetries && context.inputs.maxRetries !== 1), which will incorrectly treat 0 as “not provided” and skip installing the retry override; once MAX_RETRIES=0 parses correctly, this becomes a concrete behavior bug.
| ...((context.inputs.retryMode && context.inputs.retryMode !== "smart") || | |
| (context.inputs.maxRetries && context.inputs.maxRetries !== 1) | |
| ? { | |
| retry: { | |
| mode: context.inputs.retryMode ?? "smart", | |
| max_per_job: context.inputs.maxRetries ?? 1, | |
| eligible: [], | |
| exclude: [], | |
| }, | |
| } | |
| : {}), | |
| ...(((context.inputs.retryMode ?? "smart") !== "smart") || | |
| ((context.inputs.maxRetries ?? 1) !== 1) | |
| ? { | |
| retry: { | |
| mode: context.inputs.retryMode ?? "smart", | |
| max_per_job: context.inputs.maxRetries ?? 1, | |
| eligible: [], | |
| exclude: [], | |
| }, | |
| } | |
| : {}), |
| githubToken, | ||
| }: PrepareTagOptions): Promise<PrepareResult> { | ||
| if (isAutomationContext(context)) { | ||
| return prepareMedicMode(context, octokit, githubToken); |
There was a problem hiding this comment.
[P0] [security] Gate CI Medic workflow_run execution for untrusted PRs
The workflow_run automation path goes straight to prepareMedicMode and bypasses the existing human-actor and write-permission gates (those only run for entity contexts), which means a fork PR can trigger a secrets-bearing, tool-enabled CI Medic run in base-repo context; add an explicit gate that resolves the PR and skips unless the head repository is the base repo (or the PR author is trusted, for example has write/admin) before invoking Droid/reading logs.
| octokit, | ||
| context.repository.owner, | ||
| context.repository.repo, | ||
| run.head_branch, |
There was a problem hiding this comment.
[P1] [security] Load CI Medic config from a trusted ref (not head_branch)
loadMedicConfig(..., ref: run.head_branch, ...) reads .github/droid-ci.yml from a PR-controlled ref for same-repo PRs, which breaks the documented safety boundary that PRs cannot reconfigure CI Medic (and can be used to flip fix.enabled, tweak protected paths, or inject prompt instructions) before the PR is merged; resolve the PR first and load config from pr.baseRef (or the repo default branch), and consider treating workflow inputs like auto_fix as the hard gate regardless of config-file values.
| # Check out the head branch (not the merge commit) so CI Medic can | ||
| # reproduce failures and, when auto_fix is on, commit to a real branch. | ||
| - name: Checkout pull request branch | ||
| id: checkout_branch | ||
| continue-on-error: true | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.workflow_run.head_branch }} | ||
| fetch-depth: 0 | ||
|
|
||
| # Fork branches do not exist in the base repository. Fall back to a | ||
| # detached checkout so diagnosis still works; CI Medic downgrades to | ||
| # comment-only because it cannot push to a fork. | ||
| - name: Checkout pull request commit | ||
| if: steps.checkout_branch.outcome == 'failure' | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.workflow_run.head_sha }} | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
[P1] [security] Avoid checking out head_branch from base repo for fork PRs
Checking out ${{ github.event.workflow_run.head_branch }} without also setting repository: can silently check out the base repo’s branch when a fork PR’s branch name collides (commonly main), so CI Medic runs against the wrong code while still having access to secrets and privileged tokens; check out the exact head_sha first, and only switch to a branch checkout when the head repository matches the base repo.
| # Check out the head branch (not the merge commit) so CI Medic can | |
| # reproduce failures and, when auto_fix is on, commit to a real branch. | |
| - name: Checkout pull request branch | |
| id: checkout_branch | |
| continue-on-error: true | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.workflow_run.head_branch }} | |
| fetch-depth: 0 | |
| # Fork branches do not exist in the base repository. Fall back to a | |
| # detached checkout so diagnosis still works; CI Medic downgrades to | |
| # comment-only because it cannot push to a fork. | |
| - name: Checkout pull request commit | |
| if: steps.checkout_branch.outcome == 'failure' | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.workflow_run.head_sha }} | |
| fetch-depth: 0 | |
| # Always check out the exact head SHA so CI Medic operates on PR code. | |
| - name: Checkout pull request commit | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.workflow_run.head_sha }} | |
| fetch-depth: 0 | |
| # For same-repo PRs, also check out the branch name so CI Medic can push fixes. | |
| - name: Checkout pull request branch (same repo only) | |
| if: github.event.workflow_run.head_repository.full_name == github.repository | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.workflow_run.head_branch }} | |
| fetch-depth: 0 |
A pre-merge review found that CI Medic's security model was weaker than its documentation claimed. workflow_run executes in the base repository with write-scoped tokens, the Factory API key, and OIDC, and the model runs with permission prompts disabled, so the gaps below were reachable. Trust boundary: - Fork pull requests are no longer processed. Nothing checked the head repository anywhere, while the template asserted a comment-only downgrade that was never implemented. Enforced in the template and re-checked in the gate for hand-written workflows. - Execute, Edit, Create, and ApplyPatch are granted only when fix.enabled is on, so a diagnosis-only run cannot touch the working tree. - fix.protected_paths is enforced after the run instead of being asked for in the prompt. Violations are reverted and the job fails. - Config is read from the default branch, not the head branch. A pull request could previously grant itself auto-fix, empty its own protected paths and skip rules, and write into the agent prompt. - Job logs are labelled as untrusted data in the prompt. Correctness: - Pull requests resolve by head SHA and must still be open. Matching on branch name alone could bind a run to an unrelated pull request. - The config parser is now real YAML. The hand-rolled one dropped block sequences and any top-level key after a nested block, which silently replaced a configured protected_paths or max_runs_per_pr with the default and read as success. - The budget marker is honoured only on the app's own comments, the comment list is paginated, an unreadable list skips rather than counting as zero runs, and the marker is passed to the comment server instead of re-read, so a failed read cannot reset the count. - waitForChecksToFinish has a deadline and identifies itself by workflow name from the environment. - Action inputs no longer overwrite repository-configured arrays. - The tracking comment is rewritten when a run fails instead of reading "is analyzing" forever. - An explicit max_retries of 0 is preserved, and `?` works in globs. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The fleet caught two regressions from the hardening commit. MCP server env values are interpolated into `droid mcp add ... --env K=V` as an unquoted shell string. MEDIC_RUN_MARKER carried the assembled marker, which contains spaces and angle brackets, so registration failed three times and aborted every medic run. The run id and count are now passed as two digit-only values and the comment server rebuilds the marker from them. A test asserts no MCP env value contains whitespace or a shell metacharacter. Concurrency applies to the whole workflow, including runs whose job the condition skips, so cancel-in-progress let a passing workflow's skipped run cancel an in-flight diagnosis for a failing one. Runs are serialized per branch again instead of cancelled. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Two gaps the fleet exposed. Every watched workflow that fails raises its own workflow_run event, and the first run already waits for all checks on the commit before it reports, so the later events re-analyzed identical information and spent a budget unit each. PR 30 of the sandbox fleet finished at count=2 for a single commit. The marker now records the analyzed commit and a repeat event for it is skipped. A rerun that fails is a genuinely new outcome, so run_attempt above the first is still allowed through. The protected-path revert was never exercised end to end: it only fires when the model edits a protected file, and in the sandbox the model correctly declined, so all the fleet proved was that the model behaved. The revert is now separated from the push and tested against a real git repository, asserting the protected file is restored, an unrelated fix in the same run survives, and the restore is committed. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The fleet cannot reliably exercise this path: whether a duplicate event is cancelled while pending by the concurrency group or arrives after the first run finished is a race. Extracting the decision lets the four cases be asserted directly instead of inferred from a timing-dependent run. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Raising a second attempt-1 run on an unchanged commit is what exercises the duplicate-event gate deterministically, instead of waiting for two workflows to race and hoping the duplicate is not cancelled while pending. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
fix.scope read as an instruction in the prompt and nothing more, so a commit whose only failure was a deployment still handed the model a shell, an editor and a commit. The scope is a statement about what failed rather than about the diff, so it is now decided before Droid starts: the failing jobs for the commit are fetched, classified, and if none of them fall inside the scope the editing tools are withheld and the run is diagnosis-only. An unreadable job list is treated as an unknown scope rather than a permissive one. Categories are matched against the names repositories really use, so unit, tsc and eslint are recognized as tests, types and lint, while a category buried inside an unrelated word such as latest-release is not. An unrecognized job stays out of scope and says so in the log. This repository also had no CI. Nothing ran the 634 tests, the type checker or the formatter on a pull request, which is why this branch reported no checks. All three now run. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The spy on setupGitHubToken was never restored, and a spy on an imported module outlives the file that installed it. Any file importing that module afterwards saw the stub, so test/github/token.test.ts failed whenever the runner ordered this file first. It passed locally and failed in CI purely on file ordering. Found by adding CI, which is the point of adding CI. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The sandbox proved the gate leaked. A pull request whose only failing job was deploy-preview still received Execute, Edit, Create and ApplyPatch, because the failing step was named "Push preview bundle" and "bundle" is one of the words that identifies a build. Human-written step names are prose and matching them grants tools by coincidence, which is a gate that fails open. Only a step GitHub named itself is read now, which it does by prefixing the command with "Run", so "Run bun test" still counts and "Push preview bundle" does not. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The retry override gate used `value && value !== default`, which short-circuits on 0 (falsy). MAX_RETRIES=0 was silently treated as unset, falling through to the default of 1 retry. Changed to `(value ?? default) !== default` so 0 is honored as an explicit override. Extracted buildActionConfig for testability and added coverage for the zero, default, absent, and retryMode-off cases.
Adds CI Medic, a drop-in workflow that runs after CI completes on a pull request, diagnoses the failures, retries what looks flaky or environmental, and either commits a focused fix or posts inline suggestions.
Tracked by AUT-1522 / AUT-1523 (Dell ask for a post-CI watchdog action).
How it works
Triggered on
workflow_run, so it runs in base-repo context with access to the Actions API. It waits for every check on the head commit to finish before acting, which keeps a pull request with several failing workflows to one aggregated comment rather than one per workflow.Configuration comes from action inputs merged over an optional
.github/droid-ci.ymlread from the base branch, so a pull request cannot reconfigure the bot. Repository config is untrusted input, so every field is coerced back to its declared type and a malformed value degrades to the default.Budgets
Three separate limits, each with a distinct scope:
max_retriesmax_fix_attemptsmax_runs_per_prmax_runs_per_pris enforced at the gate, before Droid is invoked, so an exhausted budget costs nothing.Two auth details that matter
github_tokenis deliberately not passed, so the action authenticates as the Factory Droid App. GitHub does not start a new workflow run for a push made withsecrets.GITHUB_TOKEN, so an auto-fix commit would otherwise land under the stale failing checks that triggered it and never be verified. Reading job logs and rerunning jobs still use the workflow token, which is whyactions: writeis also required.The template checks out the head branch so fixes have a real branch to commit to.
CI Medic does not run on fork pull requests.
workflow_runexecutes in the base repository with write-scoped tokens, the Factory API key, and OIDC, so checking out a fork's commit and running commands against it would hand the pull request author those credentials. A job-level condition enforces this and the gate re-checks it. An earlier revision of this branch claimed forks "downgrade to comment-only"; no such downgrade existed in code, and the claim has been removed rather than implemented.Validation
scripts/ci-medic-sandbox.tsprovisions a sandbox repository and a fleet of pull requests covering each path, then asserts the outcomes.workflow_runcannot be simulated locally: the triggering definition is read from the default branch and reruns go through the Actions API.All 12 scenarios pass against a real repository: real failure with fix off and on, flaky retry, infra retry, retry disabled, two failing workflows, config failure, protected path, excluded workflow, draft, label opt-out, and budget exhaustion. Flakes are made deterministic via
GITHUB_RUN_ATTEMPTso retry assertions are not coin flips. The end-to-end loop is proven: diagnosed an off-by-one, committed the fix, CI retriggered and went green.507 unit tests pass, typecheck and format clean.
Bugs the fleet caught, fixed here
ci-medic:runmarker, but Droid rewrites that comment's body and dropped the marker, so each successful run destroyed its own record. Measured: three runs, one surviving marker.max_runs_per_prwould only ever have counted runs that crashed before Droid started. The count now lives in a marker on one reused tracking comment, which the comment server carries across rewrites. This also stops a new top-level comment appearing per run.github_inline_comment___create_inline_commentwas allowed, but the server behind it was gated onisEntityContext, which aworkflow_runpayload never satisfies, so the CLI rejected the run withUnknown tool identifier(s). That tool is how theauto_fix: falsepath delivers suggestions.workflows: { exclude: ["Deploy *"] }failedJSON.parsebecause YAML leaves keys unquoted, fell through as a string, and leftconfig.workflows.excludeundefined for a.some()call.Known caveats
action_requiredwhen the app is a first-time contributor to the repo, which stalls the verify step until someone approves. Worth surfacing in the medic comment.max_runs_per_prin that file was silently ignored, and matched the default, so the outcome was identical. The budget scenario exercised the action input, not the file.app_not_installed, and the error copy links toapps/factory-ai, a private app users cannot install.