Skip to content

Fence prepared GitHub App issuance and read only committed warm material - #2470

Closed
freeqaz-openai wants to merge 2 commits into
impl/egress-1530-integrated-send-receipts-20261014from
impl/egress-1530-prepared-github-component-20261014
Closed

freeqaz-openai wants to merge 2 commits into
impl/egress-1530-integrated-send-receipts-20261014from
impl/egress-1530-prepared-github-component-20261014

Conversation

@freeqaz-openai

@freeqaz-openai freeqaz-openai commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Superseded by native Egress Proxy stack #2488. Do not land this PR. The RFC is the base (#2484); replacement draft #2487 records this layer’s disposition. Closing without merging; branch, commits, review and evidence stay available.

DO NOT MERGE YET. Human merge required. Interface names may change.

Stack qualification hold. Do not deploy this candidate or test with live credentials. This source-only component neither changes nor clears the stack’s installed client-isolation hold.

The OpenShell compatibility test found that the current refresh bridge cannot identify distinct issuance attempts and can substitute an expiry. This adds a narrow OCE-prepared GitHub App lifecycle core, without registering a second issuer or moving every provider’s rotation into OCE.

A background owner admits the attempt before issuance, validates returned repository rights and actual provider expiry, then commits a material version. The reader only returns an eligible committed version: a cold, expired or wrong-grant read does not read the key, issue, or schedule work.

With a generated App key and local HTTPS fixture, the owner ran 128 focused tests with no skips. Independent negatives caught and fixed issuance after exact-attempt quarantine and expiry while a read was in flight. The repository fixture authenticated with the committed bytes; no live GitHub or production custody is claimed.

Next join: implement the encrypted durable family store; prove restart, takeover, withdrawal and lost acknowledgment; then connect the background worker and read-only Resolver. The current transaction fixture shares committed memory, not a production database.

Stack-tip CI repair: hosted module policy rejected importing the receipt service through concrete state/. Move the service to neutral OCC domain code; leave storage adapters and the existing policy unchanged. The exact conformance lane then passed 1,637 tests across 101 files, with no failures or skips. Hosted rerun and installed qualification are separate.

Egress Proxy RFC #1530 · Delivery and qualification gates

@clawsweeper

clawsweeper Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Oct 14, 2026
@clawsweeper

clawsweeper Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge.

What this changes

This PR adds background GitHub App token preparation and a reader that returns only eligible committed material.

Example: The admitted git-read grant has no committed token.

  • Before: There is no separate prepared-material reader.
  • After: readWarm returns not_ready without retrieving the signing key, issuing a token, or scheduling refresh.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) Useful component work remains unintegrated, with a concrete dispatch-fencing defect and insufficient final-effect authorization evidence.
Proof confidence 🧂 unranked krab (1/6) Real behavior proof is necessary before merge. See Before merge.
Patch quality 🦪 silver shellfish (2/6) Security review found an item that needs attention.

Product

Kind: Feature · Worth it: Yes · Fix scope: Partial
User problem: Agents need scoped GitHub access without receiving App keys or triggering token issuance on the request path.
Reason: The maintainer-authored direction addresses the documented bridge limitation, but this component deliberately defers the durable store and regular Agent callers.

Merge readiness

⛔ Blocked before merge - 4 items remain

Keep this PR open under its explicit qualification hold; dispatch fencing and required platform integration remain blocking.

Priority: P2
Reviewed head: d972aa72ea10352e14d17346f46ef9176d9f75a9

Before merge

  • Add real behavior proof - Authority-chain proof required: The supplied generated-key HTTPS tests exercise preparation and committed-token authentication, but the consumer is handwritten and has no production authorization. Show allowed and nearest-forbidden Namespace/grant consumers through the real sender, including withdrawal or takeover before issuance dispatch and revocation after warm selection but before provider I/O. Keep the live-credential hold and redact credentials and private endpoints. After adding proof, update the PR body, then request a new review with @clawsweeper re-review. The PR author or someone with repository write access can request it.
  • Fence current admission at provider dispatch (P1) - If withdrawal, takeover, or exact-attempt quarantine commits after readCurrent returns true but before the provider connection opens, both transport admission callbacks still succeed: assertAdmitted checks only the deadline and AbortSignal. The family mutation does not abort that signal, so GitHub receives the issuance POST under withdrawn authority. The later publication transaction quarantines the token but cannot prevent the already-created provider credential. Bind dispatch to a live admission fence or coordinated cancellation, preserving the transport's existing reject-before-send contract.
  • Connect the preparation core to the supported Agent workflow (P2) - The new owner and reader are exported but called only by the added tests, and GitHubPreparationStore has no production implementation. Consequently, ordinary Agent execution cannot prepare or consume this material. AGENTS.md explicitly requires platform lifecycle/composition integration, regular Agent callers, and integration coverage through those callers; the body instead defers all three. Complete that join before landing this capability, or retain this as an unlanded draft until the integration is ready.
  • Resolve security concern: Withdrawn issuance authority survives connection setup - This crosses the OCC admission-to-provider-effect boundary; quarantining the returned token prevents publication but does not undo issuance.

Findings

  • [P1] Fence current admission at provider dispatch — apps/controller/src/drivers/repo/github/credentials/prepared-owner.ts:104-105
  • [P2] Connect the preparation core to the supported Agent workflow — apps/controller/src/drivers/repo/github/credentials/prepared-owner.ts:21-26
  • [medium] Withdrawn issuance authority survives connection setup — apps/controller/src/drivers/repo/github/credentials/prepared-owner.ts:105

Tests

  • Missing end-to-end proof: Missing evidence covers withdrawal between admission read and dispatch, forbidden or revoked consumers at final provider I/O, and the regular Agent workflow; no base-fail/head-pass result was supplied for those scenarios.
Agent review details

How this fits together

The component sits inside the controller's GitHub repository credential implementation, taking trusted grant admission and signing-key access and producing committed tokens for a future Resolver.

flowchart TB
  A[Trusted admission] --> B[Preparation owner]
  K[App signing key] --> B
  B --> G[GitHub token endpoint]
  B --> S[Family store port]
  S --> R[Warm reader]
  W[Background worker] -. pending integration .-> B
  R -. pending integration .-> C[Authorized Resolver]
Loading

Technical review

Best possible solution:

Integrate one durable preparation owner and authorized warm consumer, preserving the hold until withdrawal prevents unauthorized dispatch and the supported workflow is qualified.

Do we have a high-confidence way to reproduce the issue?

Source shows that withdrawal after readCurrent succeeds but before TCP connect leaves observeDispatch checking only time and cancellation; this path was not executed during review.

Is this the best way to solve the issue?

Reusing the existing signer, transport, and response validator is appropriate, but a detached admission snapshot cannot replace dispatch fencing or regular workflow integration.

Full review comments:

  • [P1] Fence current admission at provider dispatch — apps/controller/src/drivers/repo/github/credentials/prepared-owner.ts:104-105
    If withdrawal, takeover, or exact-attempt quarantine commits after readCurrent returns true but before the provider connection opens, both transport admission callbacks still succeed: assertAdmitted checks only the deadline and AbortSignal. The family mutation does not abort that signal, so GitHub receives the issuance POST under withdrawn authority. The later publication transaction quarantines the token but cannot prevent the already-created provider credential. Bind dispatch to a live admission fence or coordinated cancellation, preserving the transport's existing reject-before-send contract.
    Confidence: 0.94
  • [P2] Connect the preparation core to the supported Agent workflow — apps/controller/src/drivers/repo/github/credentials/prepared-owner.ts:21-26
    The new owner and reader are exported but called only by the added tests, and GitHubPreparationStore has no production implementation. Consequently, ordinary Agent execution cannot prepare or consume this material. AGENTS.md explicitly requires platform lifecycle/composition integration, regular Agent callers, and integration coverage through those callers; the body instead defers all three. Complete that join before landing this capability, or retain this as an unlanded draft until the integration is ready.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 2e09588e05c3.

Provenance checked

Testing

Proof path: in-process harness.

Security

Needs attention: Durable withdrawal does not currently invalidate the new owner's authority at provider dispatch.

Evidence

Security concerns:

  • [medium] Withdrawn issuance authority survives connection setup — apps/controller/src/drivers/repo/github/credentials/prepared-owner.ts:105
    This crosses the OCC admission-to-provider-effect boundary; quarantining the returned token prevents publication but does not undo issuance.
    Confidence: 0.94

What I checked:

Likely related people:

  • Free Wortley: Raw commit f284d34 adds apps/repository-credentials/src/backends/github/driver/acquisition-response.ts:86 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: f284d342fe8e; files: apps/repository-credentials/src/backends/github/driver/acquisition-response.ts)
  • Mrunal Patel: Raw commit 6d9054f adds docs/flows/credential-source-refresh.md:13 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 6d9054f5d096; files: docs/flows/credential-source-refresh.md)

Labels

Label changes:

  • add P2: This is an unregistered draft component with consequential credential-boundary defects, without evidence of a released-user incident.
  • add merge-risk: 🚨 security-boundary: The new owner can dispatch GitHub token issuance after durable admission has been withdrawn during connection setup.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Label justifications:

  • P2: This is an unregistered draft component with consequential credential-boundary defects, without evidence of a released-user incident.
  • merge-risk: 🚨 security-boundary: The new owner can dispatch GitHub token issuance after durable admission has been withdrawn during connection setup.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 14, 2026, 6:23 PM ET / 22:23 UTC.

This branch had an error being deployed

1 failed deployment
integration-qa-pr — da7d9743 Deployed Oct 14, 2026 by freeqaz-openai via QA advisory (compose) #966
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant