Skip to content

docs(#6578): add vendored dependency checklist - #7267

Open
shairevivo wants to merge 6 commits into
fullsend-ai:mainfrom
shairevivo:docs/6578-vendored-dependency-checklist
Open

shairevivo wants to merge 6 commits into
fullsend-ai:mainfrom
shairevivo:docs/6578-vendored-dependency-checklist

Conversation

@shairevivo

@shairevivo shairevivo commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a contributor-facing checklist for adding or bumping sandbox-vendored dependencies. It consolidates the auth, artifact integrity, supply-chain, permissions, environment, egress, and runtime evidence that #6572 established after an OAuth-based extension failed in a fresh sandbox.

Closes #6578

Validation

  • make lint (staged documentation change)
  • make lint-md-links
  • Independent, read-only proposal review completed before implementation.

Vendored-dependency checklist evidence

This PR changes documentation only; no dependency is added or bumped.

The checklist's requirement to record each applicable item as verified, blocked, or not applicable deliberately operationalizes #6578's validation criteria: future vendoring PRs should reference the checklist and flag anything they cannot verify. This PR keeps that evidence requirement in the contributor runtime guide; it does not add broader PR-template or repository-wide enforcement.

  • Fresh-sandbox auth: not applicable
  • Artifact SHA256: not applicable
  • Dependency/release review: not applicable
  • Sandbox ownership and permissions: not applicable
  • Environment-variable audit: not applicable
  • Egress compatibility: not applicable
  • End-to-end and co-install regression: not applicable

Re-review changes: required evidence

  • Producer/consumer matrix: not applicable — no forge, dispatch, or cross-repository contract changes.
  • Instruction surfaces: verified — the contributor runtime guide's table of contents and pinned-binary guidance both point to the checklist.
  • Adversarial re-review scenario: not applicable — no re-review dispatch behavior changed.
  • Historical constraints: verified — feat(#6571): add xai-vertex provider to the pi runtime #6572's fresh-sandbox OAuth failure, independent digest recomputation, permission hardening, and co-installed extension regression informed the checklist.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Shai Revivo <srevivo@redhat.com>
@shairevivo
shairevivo requested a review from a team as a code owner September 13, 2026 17:55
@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Document sandbox vendored dependency verification checklist

📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Adds a required checklist for sandbox-vendored dependency additions and version bumps.
• Covers authentication, integrity, supply-chain, permissions, environment, egress, and runtime
 validation.
• Requires PRs to record each checklist item’s verification status.
Diagram

graph TD
  A["Select version"] --> B["Verify authentication"] --> C["Validate artifact"] --> D["Review supply chain"] --> E["Harden sandbox"] --> F["Confirm egress"] --> G["Run regressions"] --> H["Record evidence"]
Loading
High-Level Assessment

The chosen approach is appropriate: placing the checklist beside the existing pinned-runtime guidance keeps dependency requirements discoverable and contextual, while the table-of-contents entry provides direct navigation. A separate policy document would add indirection without improving this focused contributor workflow.

Files changed (1) +15 / -0

Documentation (1) +15 / -0
runtime-implementation.mdAdd sandbox vendored dependency checklist +15/-0

Add sandbox vendored dependency checklist

• Adds a seven-step checklist for adding or bumping sandbox dependencies, covering authentication, artifact integrity, supply-chain review, filesystem permissions, environment handling, egress, and regression testing. It also requires contributors to record each item as verified, blocked, or not applicable and links the section from the page contents.

docs/contributing/runtime-implementation.md

@codecov

codecov Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

Site preview

Preview: https://ccb99fee-site.fullsend-ai.workers.dev

Commit: 89c98613c611297cd89c14a2b4624f7a905de37a

@ifireball ifireball added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Sep 15, 2026
@ifireball

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 15, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:53 AM UTC · Completed 9:09 AM UTC

Commit: b9ed0be · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.25

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 15, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 15, 2026

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Anchored to the prior score of 2 (moderate): Tier 1 signals are unchanged from the prior run (single-file, ~16-line docs-only diff, no protected/security/CI/dependency touches, non-bot non-first-time author); Tier 2 reconfirms this doc is a known high-churn, fix/revert-heavy hotspot already priced into the prior score, while Tier 3 shows the PR scope cleanly matches its linked issue with no risk labels, weighting the composite down to a rounded 2 (moderate).

Previous run

Risk Assessment: moderate (2/5)

Details

Score unchanged from the prior review (2/moderate): independent re-verification confirms Tier 1 signals are identical (single-file, 16-line docs-only diff, no protected/security/CI/dependency touches, non-bot author) and the file under review is byte-identical to the prior-reviewed revision — the only intervening commits are an unrelated main merge (ADR 0106/coalescing docs from PR #6909). Tier 2 reconfirms the same known high-churn, fix/revert-heavy documentation hotspot already priced into the prior score, with no new risk signal introduced.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Score unchanged from the prior review (2/moderate): Tier 1 signals are identical in shape (single-file, 16-line docs-only diff, no protected/security/CI/dependency touches, returning non-bot author), and Tier 2 confirms the same high-churn/fix-revert documentation hotspot already reflected in the prior score, with the amendment being a minor wording/placement refinement to the same file rather than a scope or risk-signal change.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Small (1 file, 15 lines), docs-only, additive checklist addition by a returning contributor that closely satisfies its linked, unlabeled issue, with no protected/security/CI/dependency touches; the only elevating signal is that the target file is a high-churn documentation hotspot, nudging the score from low to moderate.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 15, 2026

Copy link
Copy Markdown

Looks good to me

Previous run

Review

Findings

Low

  • [sub-agent-failure] N/A — The challenger sub-agent returned an empty adjudicated-findings array while the input set (2 findings) was non-empty — it removed both findings rather than adjudicating them. Per orchestrator policy this is treated as a challenger failure, so the pre-challenger finding set (both info-level, filtered below the posting threshold) is used instead.
Previous run (2)

Review

Re-review: both prior findings (imprecise 0555 permission claim; checklist section misplaced ahead of the pinned-binaries table with a non-conforming standalone TOC entry) are fixed in this revision — item 4 now correctly distinguishes 0555 (directories/executables) from 0444 (ordinary files) matching the Containerfile's chmod -R a+rX,a-w / chmod a+rX,a-w sequence, and the checklist section now sits after the pinned-binaries table with its TOC entry folded inline into the parent bullet.

Findings

Low

  • [scope-authorization] docs/contributing/runtime-implementation.md:404 — Linked issue Add vendored-dependency sandbox-compatibility checklist to runtime implementation guide #6578 proposed a checklist with 6 example items ("items such as: ...") consolidating guidance already present in the document. The PR ships 7 items (adding an egress-compatibility check not enumerated in the issue's list, though it mirrors the document's existing "Egress binary identity per runtime" section) plus a closing, unnumbered sentence that introduces a new contributor-facing requirement: every applicable checklist item must be recorded in the PR description as verified, blocked, or not applicable. That recording requirement reads as a new process/compliance convention rather than pure reference material, and it is not wired into CONTRIBUTING.md or a PR template, so nothing enforces it beyond social convention within this PR body.
    Remediation: If the recording requirement is intentional (it does operationalize the issue's own validation criteria, which asks the next vendoring PR to reference the checklist and flag unverifiable items), consider noting explicitly in the PR description that this is a deliberate operationalization of Add vendored-dependency sandbox-compatibility checklist to runtime implementation guide #6578's validation criteria, and/or file a follow-up to wire the requirement into CONTRIBUTING.md or a PR template so it is discoverable and enforceable beyond this single PR's prose.

  • [sub-agent-failure] N/A — The challenger sub-agent returned an empty adjudicated-findings array while the input set (2 findings) was non-empty — it removed both findings rather than adjudicating them. Per orchestrator policy this is treated as a challenger failure, so the pre-challenger finding set is used below instead.

Info

  • [architectural-fit] docs/contributing/runtime-implementation.md:391 — The checklist content (fresh-sandbox auth, SHA256 verification, permission hardening, env-var scrubbing, egress allowlisting, end-to-end regression) accurately traces to and consolidates guidance already present elsewhere in the same document, and to the historical incident in feat(#6571): add xai-vertex provider to the pi runtime #6572/Add xai-vertex provider to the pi runtime (Grok 4.6 on Vertex AI) #6571 cited in the PR body. It is organization-agnostic, docs-only, and fits the existing "Pinned runtime binaries in the sandbox image" section placement and TOC structure — no architectural mismatch found.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

Review

Findings

Low

  • [logic-error] docs/contributing/runtime-implementation.md:388 — Item 4 of the new "Vendored dependency checklist" states that current pi extensions use mode 0555. images/sandbox/Containerfile never sets 0555 directly; it runs chmod -R a+rX,a-w "${PI_EXTENSIONS_DIR}/<ext>" on each extension subtree (lines 164, 203) and chmod a+rX,a-w "${PI_EXTENSIONS_DIR}" on the parent (line 204). That produces 0555 for directories and for files that already carried an execute bit, but 0444 for ordinary non-executable regular files (verified: chmod 644 file; chmod 755 dir/exec; chmod -R a+rX,a-w . → dir=555, file.txt=444, exec.sh=555). A contributor who copies "0555" literally, or who stats a plain data file in the extension tree, will see 0444 and may incorrectly conclude the build failed the checklist. The item's actual gate ("root-owned and unwritable to the sandbox user") is already the right criterion; only the octal example is imprecise.
    Remediation: Replace the literal "0555" claim with a description of the actual command (chmod -R a+rX,a-w on the extension subtree, plus a non-recursive chmod a+rX,a-w on the parent directory for the last-installed extension), and describe the effective boundary as "root-owned and unwritable to the sandbox user" rather than a single octal mode, since directories/executables land on 0555 but plain files land on 0444.

  • [document-structure] docs/contributing/runtime-implementation.md:381 — The new ### Vendored dependency checklist H3 is inserted between the pinned-binaries lead-in ("What is pinned, and what to re-check when a pin moves:") and the 3-column table that sentence introduces, so the colon no longer precedes the table whose columns it names (Binary / Pin / Re-check on bump). Separately, the new H3 is given its own top-level "On this page" TOC bullet; no other H3 in this file gets a standalone TOC entry (the closest precedent, "Sandbox hook contract," folds its H3 topics into the parent bullet as inline text after an em dash instead).
    Remediation: Move the ### Vendored dependency checklist subsection to after the pinned-binaries table and its explanatory notes (e.g., immediately before ## Egress binary identity per runtime), so the lead-in sentence and the table it introduces remain adjacent. Optionally fold the new TOC entry into the existing "Pinned runtime binaries in the sandbox image" bullet as inline descriptive text instead of a separate bullet, unless the checklist is promoted to its own H2.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

Keep the pin table adjacent to its lead-in and document permission modes.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Shai Revivo <srevivo@redhat.com>
@github-actions github-actions Bot removed the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Sep 15, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:28 AM UTC · Completed 3:41 AM UTC

Commit: e7e6dac · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.47

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:16 AM UTC · Completed 8:30 AM UTC

Commit: bc94989 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.22

@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself September 16, 2026 08:30

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Sep 16, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:10 PM UTC · Completed 9:21 PM UTC

Commit: 89c9861 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add vendored-dependency sandbox-compatibility checklist to runtime implementation guide

2 participants