mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 08:22:20 +02:00
fix(native): honor duplex transport for GitHub callback bridges (#16455)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Native remote runners use a session-owned broker for managed GitHub operations. > - That broker uses the existing authenticated sandbox callback bridge. > - The native caller omitted the server-selected duplex flag. An enabled and capable target still used file polling. > - Warm sessions must observe a changed flag and channel loss before the next admission. > - This pull request forwards the existing flag and retires unavailable or changed bridges before retained instruction preparation. > - The benefit is less idle remote traffic without changing credential authority or the default transport. ## Linked Issues or Issue Description **What happened?** A native GitHub callback bridge selected `queue_v1` even when its sandbox target had duplex enabled and supported it. The file bridge continues remote queue checks while the warm session remains available. **Expected behavior** An enabled, capable target should use the existing HTTP/2 bridge. Disabled or unsupported targets and failed channel startup must retain the authenticated file fallback. A changed server flag must take effect at the next normal warm-session boundary. **Steps to reproduce** 1. Create native GitHub access on a sandbox target with the server-selected duplex flag enabled, verified duplex capability and a working channel. 2. Observe the selected callback transport. 3. Before this fix, it is `queue_v1`. The added real-wire regression expects `http2_v1` and fails on the old source. ## What Changed - Forward the existing server-selected duplex flag through the native GitHub broker. - Record that transport selection on the broker. Retire the warm provider and broker through the existing cleanup path when the flag changes. - Read live channel failure when checking broker readiness. Mark normal session cleanup as an orderly completion. - Include the selected flag in instruction preparation identity. Retire a failed broker before adopting retained instructions, while preserving existing ownership and cleanup fences. - Add real local HTTP/2 credential-rotation and file-fallback controls. Add native warm-session flag-change controls. - Document selection, fallback and warm-session behavior. - Scan CI secret expressions in one forward pass, including quoted braces and escaped apostrophes. Preserve direct, bracket, multiline and whole-context references while rejecting filename and literal-string false positives. ## Verification - Current head `41440b07ea45e9e882e36ed495a66e501c8dd4df` resolves the actual CI-policy conflict with master `307eb16c0c59abd9e0626fde9201939b262e7f5f`. It retains upstream's stricter whole-expression credential guard, including quoted secret text, and uses the forward scanner instead of a backtracking regex. All **16 CI policy tests** pass, including the bounded 28- and 10,000-apostrophe controls in both policy modes. The reported old regex still has its recorded two-second timeout (14 passed, one failed). On this final merged source, **809 affected native cases** pass with zero selected skips (65.20s). Full repository typecheck (31.44s) and build (22.13s) pass. Frozen source hashes stayed unchanged. Fresh exact-head full CI, configured security and completed Apex remain pending. - Before master integration, the frozen native repair passed repository `pnpm -r typecheck` passed (33.0s) and `pnpm build` passed (32.1s). - Before the latest master integration, all **800 affected cases passed with zero selected skips**: complete native executor (725), resume (50), GitHub broker (16), environment duplex-gate (4), and heartbeat instruction-copy (5) suites. The final run took 95.3s. Source hashes stayed unchanged through these checks. - The initial head passed 788 affected cases and full CI, but Apex scored it **3/5**. It found a dead HTTP/2 broker could remain reusable and a flag change could fail after retained instruction preparation. Both findings required the new repair. Three added before-fix regressions failed on channel availability, orderly close, and preparation identity. - The first repair run failed three new warm-instruction fixtures because fake provider close left its recorded state ready. The corrected fixture writes a suspended checkpoint without weakening the production ownership guard. A separate real-wire run then failed two availability assertions because its helper spread had frozen the readiness getter. The helper now preserves the getter. All failed attempts remain in the local evidence. - One disposable real Daytona sandbox exercised both transports on the initial transport patch through the production native broker and callback bridge. The provider shell wrapper used the then-current production declarations. During equal two-second idle windows, file mode issued **9 remote commands** and HTTP/2 issued **0**. Both variants rotated synthetic credentials between two run bindings and denied idle requests. Cleanup deleted the sandbox. - Daytona component setup took 3.64s for file mode and 4.74s for HTTP/2. This is an idle-traffic reduction, not a startup-speed claim. That component run predates the availability and preparation repair; it does not qualify the repaired paths on Daytona. The new failure controls use a real local HTTP/2 channel. The credential resolver was synthetic and no model was called. Real GitHub-account grants, staging replies, and the end-to-end latency target remain unverified by this component test. - Repaired-head CI found a policy-test false positive: a broad `secrets.` regex matched an upstream filename. The unchanged test reproduced the failure, and the corrected version passed all 15 controls. Master later supplied a stronger direct/bracket check, which we retained when resolving the merge conflict. Review then found that formatted expressions could stop that check at a quoted brace. The new reported-case baseline failed (one failed, 14 passed). The final check treats quoted strings as complete tokens, respects expression boundaries, and passes all 15 cases. The later master conflict also added `toJSON(secrets)` controls. The old branch pattern failed that whole-context case. Review subsequently found exponential backtracking with escaped apostrophes; the final forward scanner retains the quoted-boundary and whole-context controls and passes all 15 tests, including the bounded long-expression regression. No workflow or credential handling changed. All failed attempts remain recorded. - The full local repository test suite was not rerun. [Prior-head full CI](https://github.com/paperclipai/paperclip/actions/runs/38775801821), including `ci / verify` and `ci / e2e`, passed on previous head `e5f8bdb425081cf08110c0f7423f96609e405920`. All 52 check runs succeeded, two optional Storybook jobs skipped, and legacy Snyk passed. Completed Apex review is **5/5** on that exact head, with no unresolved review threads. ## Risks - Only an explicit server-selected opt-in can use HTTP/2. The existing provider-capability, channel-startup and authenticated file-fallback checks remain in the shared bridge. - Toggling the flag or losing the HTTP/2 channel replaces an idle retained provider at the next run boundary. Instruction preparation must make the same retirement decision. Normal ownership, settled history and cleanup checks remain in effect. - Lower idle traffic may reduce provider/controller pressure, but no staging latency improvement is claimed. This feature remains inactive when the existing flag is off. Other preparation and finalization costs remain. - No schema, API, UI, credential-selection, tracing endpoint or default-setting changes. ## Model Used - OpenAI Codex, based on GPT-6, with tool-assisted reasoning and code execution. The exact hosted snapshot ID and context-window size are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
307eb16c0c
commit
3898f6a45f
8 files changed
+273
-32
No files matched your search
@@ -232,19 +232,56 @@ test("export paces batches and retries only rejected transient events", async ()
|
||||
await assert.rejects(sendEvents([events[0]], "test-only", "paperclip-ci", async () => ({ ok: true, json: async () => [{ status: 400 }] })), /status 400/);
|
||||
});
|
||||
|
||||
function assertNoPrWorkflowCredentials(source) {
|
||||
assert.doesNotMatch(source, /HONEYCOMB.*KEY/);
|
||||
for (const [expression] of source.matchAll(/\$\{\{(?:'(?:[^']|'')*'|(?!\}\})[\s\S])*\}\}/g)) {
|
||||
assert.doesNotMatch(expression, /\bsecrets\b/);
|
||||
function secretReference(text, includeQuoted = false) {
|
||||
for (const line of text.split(/[\r\n\u2028\u2029]/)) {
|
||||
const start = line.indexOf("HONEYCOMB");
|
||||
if (start >= 0 && line.indexOf("KEY", start + "HONEYCOMB".length) >= 0) return true;
|
||||
}
|
||||
let offset = 0;
|
||||
while (offset < text.length) {
|
||||
const start = text.indexOf("${{", offset);
|
||||
if (start < 0) return false;
|
||||
offset = start + 3;
|
||||
let quoted = false;
|
||||
let expression = "";
|
||||
while (offset < text.length) {
|
||||
const char = text[offset];
|
||||
if (char === "'") {
|
||||
if (quoted && text[offset + 1] === "'") {
|
||||
offset += 2;
|
||||
continue;
|
||||
}
|
||||
quoted = !quoted;
|
||||
expression += includeQuoted ? char : " ";
|
||||
} else if (!quoted) {
|
||||
if (char === "}" && text[offset + 1] === "}") {
|
||||
offset += 2;
|
||||
break;
|
||||
}
|
||||
expression += char;
|
||||
} else if (includeQuoted) {
|
||||
expression += char;
|
||||
}
|
||||
offset += 1;
|
||||
}
|
||||
if (/\bsecrets\b/.test(expression)) return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
function assertNoPrWorkflowCredentials(source) {
|
||||
assert.equal(secretReference(source, true), false);
|
||||
}
|
||||
|
||||
test("PR credential guard permits filenames but rejects secret expressions", () => {
|
||||
assertNoPrWorkflowCredentials("run: node --test scripts/docker-compose-secrets.test.mjs");
|
||||
for (const source of ["env: ${{ secrets.EXAMPLE }}", "env: ${{ secrets['EXAMPLE'] }}", "env: ${{ toJSON(secrets) }}", "env: ${{ format('}}', secrets.EXAMPLE) }}"]) {
|
||||
for (const source of ["env: ${{ secrets.EXAMPLE }}", "env: ${{ secrets['EXAMPLE'] }}", "env: ${{ toJSON(secrets) }}", "env: ${{ format('}}', secrets.EXAMPLE) }}",
|
||||
"# Expressions start with '${{'.\nTOKEN: ${{ secrets.PR_TOKEN }}",
|
||||
"name: Example # Expressions start with '${{'.\nTOKEN: ${{ secrets.PR_TOKEN }}"]) {
|
||||
assert.throws(() => assertNoPrWorkflowCredentials(source));
|
||||
}
|
||||
assert.throws(() => assertNoPrWorkflowCredentials("HONEYCOMB_API_KEY: example"));
|
||||
assert.throws(() => assertNoPrWorkflowCredentials("${{ format('secrets.EXAMPLE', 'literal') }}"));
|
||||
});
|
||||
|
||||
test("workflow exporter runs trusted code and keeps keys out of the PR jobs", () => {
|
||||
@@ -254,6 +291,35 @@ test("workflow exporter runs trusted code and keeps keys out of the PR jobs", ()
|
||||
assert.match(workflow, /environment: ci-profiling/);
|
||||
assert.doesNotMatch(workflow, /head_sha|secrets: inherit|contents: write|pull-requests: write|pnpm install/);
|
||||
const pr = readFileSync(new URL("../../workflows/pr-trusted.yml", import.meta.url), "utf8");
|
||||
assert.equal(secretReference("run: node --test scripts/docker-compose-secrets.test.mjs"), false);
|
||||
for (const lookup of ["${{ secrets.PR_TOKEN }}", "${{ secrets['PR_TOKEN'] }}", "${{\n secrets.PR_TOKEN\n }}", "${{ toJSON(secrets) }}", "${{ secrets.HONEYCOMB_API_KEY }}"]) {
|
||||
assert.equal(secretReference(lookup), true);
|
||||
}
|
||||
for (const lookup of [
|
||||
"${{ format('Bearer {0}', secrets.PR_TOKEN) }}",
|
||||
"${{ format('{0}', toJSON(secrets)) }}",
|
||||
"${{ format('{{Bearer}} {0}', secrets['PR_TOKEN']) }}",
|
||||
"${{ format('it''s {0}',\n secrets.PR_TOKEN) }}",
|
||||
]) {
|
||||
assert.equal(secretReference(lookup), true);
|
||||
}
|
||||
for (const nonSecret of [
|
||||
"${{ format('secrets.PR_TOKEN', 'literal') }}",
|
||||
"${{ github.sha }}\nrun: node --test scripts/docker-compose-secrets.test.mjs",
|
||||
]) {
|
||||
assert.equal(secretReference(nonSecret), false);
|
||||
}
|
||||
const escapedExpressions = [28, 10_000].flatMap(count => {
|
||||
const expression = "${{ format('" + "it''s ".repeat(count) + "{0}', github.sha) }}";
|
||||
return [expression, expression.replace("github.sha", "secrets.PR_TOKEN"), expression.replace("github.sha", "toJSON(secrets)")];
|
||||
});
|
||||
const boundedScan = spawnSync(process.execPath, ["-e", `const fs=require('node:fs');const scan=${secretReference.toString()};process.stdout.write(JSON.stringify(JSON.parse(fs.readFileSync(0,'utf8')).map(value=>[scan(value),scan(value,true)])));`], {
|
||||
input: JSON.stringify(escapedExpressions),
|
||||
encoding: "utf8", timeout: 2_000,
|
||||
});
|
||||
assert.equal(boundedScan.status, 0, boundedScan.error?.message);
|
||||
assert.deepEqual(JSON.parse(boundedScan.stdout), [[false, false], [true, true], [true, true], [false, false], [true, true], [true, true]]);
|
||||
assert.equal(secretReference(pr), false);
|
||||
assertNoPrWorkflowCredentials(pr);
|
||||
assert.match(pr, /if: always\(\) && env.PAPERCLIP_CI_PROFILE == 'true'/);
|
||||
assert.doesNotMatch(pr, /PAPERCLIP_CI_PROFILE:.*labels/);
|
||||
|
||||
Reference in new issue
Block a user