Skip to content

fix(sandbox): render the default branch into platform contracts - #186

Open
oskurikhin wants to merge 1 commit into
theam:mainfrom
oskurikhin:fix/platform-contract-default-branch
Open

fix(sandbox): render the default branch into platform contracts#186
oskurikhin wants to merge 1 commit into
theam:mainfrom
oskurikhin:fix/platform-contract-default-branch

Conversation

@oskurikhin

Copy link
Copy Markdown

What's wrong

renderRunContract() substitutes two of the three placeholders that exist in the
shared prompt templates. The third, {{DEFAULT_BRANCH}}, reaches platform-lane
agents literally.

It lands inside the CI-doctor's <security_audit_gate>
packages/cli/templates/prompts/doctor.md:31:

Before editing, check the failure and the PR's changed files
(`git diff --name-only origin/{{DEFAULT_BRANCH}}...HEAD`). STOP without code
changes — and say why in your PR comment — if anything touches:
`.github/workflows/`, `.github/facility/`, secrets or `.env*`, auth or access
control, migrations, dependency lockfiles, `guards/`, or the doctor policy itself.

So the agent is instructed to run git diff --name-only origin/{{DEFAULT_BRANCH}}...HEAD,
which is not a valid ref. The gate's stop-list depends on that diff to know which
paths the pull request touches.

The repository lane is unaffected: it renders the same templates through
packages/core/src/render.ts, which resolves every placeholder.

How it reaches an agent

  • prompts/doctor is a registered platform contract —
    services/api/src/routes/v1/projects-repos.ts:503
  • seeding stores template content verbatim — packages/db/src/seed.ts:761
  • dispatch calls renderRunContract(...) at
    services/api/src/sandbox/orchestrator.ts:1731, inside the same function that
    handles ci_doctor mode
  • the runner performs no placeholder substitution of its own

The comment above renderRunContract already states the intent — "so agents never
receive literal {{...}} instructions"
. The body implements two thirds of it.

What this changes

repo is already selected in full at orchestrator.ts:1691, and
repos.default_branch is notNull, so the value was already in scope. This passes
it through and adds the third substitution, falling back to main.

The test worth looking at is the third one. It reads
packages/cli/templates/prompts/*.md, renders each through renderRunContract, and
fails if any {{...}} survives. The platform renderer is a hand-written subset of the
repo-lane engine, so this turns the next added placeholder into a red build instead of
a silent instruction. With the production change reverted it reports:

AssertionError: expected [ '{{DEFAULT_BRANCH}}' ] to deeply equal []

Verification

pnpm --filter @facility/api exec vitest run test/orchestrator-checks.test.ts
# Test Files 1 passed (1) — Tests 24 passed (24)

pnpm --filter @facility/api exec vitest run test/sandbox.test.ts \
  test/sandbox-capabilities.test.ts test/sandbox-cache.test.ts \
  test/docker-sandbox.test.ts test/ci-doctor-policy.test.ts
# Test Files 5 passed (5) — Tests 37 passed | 1 skipped (38)

pnpm --filter @facility/api exec tsc --noEmit    # clean
pnpm exec biome lint <the two changed files>     # clean

I could not run the full pnpm verify ladder here. The database-backed tier
self-skips without Postgres on this machine, so github-platform-lane.test.ts and the
other DB-gated suites were skipped rather than run — CI covers them. biome check also
reports a formatter diff on both files, but it does so on untouched files too
(services/api/src/previews.ts, executors.ts, learning.ts each report one): it is
the CRLF artifact of a Windows checkout with core.autocrlf=true, and the committed
content is LF.

A design question I did not decide here

This keeps the hand-written subset and guards it with a test. The alternative is to have
the platform lane reuse packages/core/src/render.ts outright, which removes the second
renderer instead of testing it. That is a larger change and felt like yours to make —
happy to follow up with it if you would rather go that way.

renderRunContract substituted two of the three placeholders present in the
shared prompt templates. The third, {{DEFAULT_BRANCH}}, reached platform-lane
agents literally, including inside the CI-doctor's security_audit_gate, where
it forms the command that enumerates a pull request's changed files.

The repository lane is unaffected: it renders the same templates through
packages/core/src/render.ts, which resolves every placeholder.

The connected repository's default branch was already in scope at the call
site, so pass it through. Add a test that renders every shared prompt template
and fails on any placeholder the platform renderer leaves behind, so the next
one breaks the build instead of reaching an agent.

@adrian-lorenzo adrian-lorenzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution!

defaultBranch comes from GitHub, and valid Git ref names may contain shell metacharacters. This substitution places it unquoted inside a command the CI doctor is told to run, so a hostile default branch can produce executable shell injection in the sandbox. Please avoid interpolating the value into shell text or quote it safely, and add a regression test using a hostile but valid Git ref.

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.

2 participants