Fix stack 14: example agent — opt-in/scrubbed candidate install, --draft-pr fix#38
Merged
ebarti merged 4 commits intoJul 2, 2026
Merged
Conversation
ebarti
force-pushed
the
review-fixes/13-docs-policy
branch
from
1e865fb to
9a106ca
Compare
ebarti
force-pushed
the
review-fixes/14-example-safety
branch
2 times, most recently
from
110761f to
cea9222
Compare
ebarti
force-pushed
the
review-fixes/13-docs-policy
branch
2 times, most recently
from
22b065c to
e1fcdb3
Compare
ebarti
force-pushed
the
review-fixes/14-example-safety
branch
2 times, most recently
from
962da19 to
8839fc3
Compare
…r crash Safety fixes for the sdk-evolution example agent: - Candidate inspection pip-installed and imported the latest upstream SDK from PyPI on every default run (arbitrary code execution from freshly published code). It is now opt-in: --inspect-candidates defaults to False (and the RunOptions default too), and _collect_snapshots honors the flag instead of treating inspection as mandatory. When it does run, the venv/pip/import subprocesses get a scrubbed environment (throwaway HOME, PATH only) so a malicious candidate cannot read the caller's credentials/config. - The default --draft-pr flow raised RuntimeError finalizing the report because the default report dir is gitignored, so 'git add' of it returned rc=1. The finalize step now skips cleanly when the report path is gitignored (the report is already embedded in the PR body), and _create_autonomous_pr stages only uv.lock. Updates the example tests to the opt-in default and adds tests: default run does not install candidates; finalize skips a gitignored report dir. Refs REVIEW.md §3.1, §8.
…ng kinds FixtureEvolutionRuntime and the test RecordingRuntime now implement aclose/__aenter__/__aexit__, and example code labels runtimes via runtime_kind_value() instead of .kind.value, which raises AttributeError for namespaced third-party string kinds.
…t gitignored ones git check-ignore exits 128 for a path outside the repository; treating that as 'not ignored' staged a doomed out-of-repo path and raised. Only a definitively not-ignored path (exit 1) is now committed. Adds the rc=128 test, pins that a drifted lock without --inspect-candidates never installs candidates, and documents the proxy/CA trade-off of the scrubbed env.
… links to github.com Three gaps in the example's handling of untrusted upstream content: - Behavior probes installed and imported candidate SDK versions unconditionally, with the caller's full environment — bypassing both the --inspect-candidates opt-in and the credential scrub the API snapshots already apply. The probes now honor the same gate, share snapshots.isolated_env (throwaway HOME, PATH only) for all three subprocesses, record skipped candidates explicitly, and a one-sided skip is excluded from diffs so the gate cannot read as a breaking change. Without the opt-in, a drifted lockfile baseline falls back to probing the installed environment. - Release-note links are extracted from fetched pages that can carry user-generated markup; a protocol-relative href (//evil.example/x/discussions/1) survived the regex and urljoined into an off-site GET. Followed links must now resolve to https://github.com. - A configured GitHub token whose GraphQL call fails (expired, insufficient scope) was silently swallowed into the unauthenticated HTML scrape. It now surfaces on the source record; tokenless runs still use the plain fetch as their normal mode. Docs updated: candidate inspection is documented as opt-in (the "always enabled" claim was stale) and the design doc records the inverted default.
ebarti
force-pushed
the
review-fixes/14-example-safety
branch
from
8839fc3 to
3a0e585
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked review-fix PR 14/N (base:
review-fixes/13-docs-policy). Final PR in the stack.REVIEW.md §3.1 (HIGH), §8:
--inspect-candidates(and theRunOptionsdefault) default toFalse, and_collect_snapshotshonors the flag instead of forcing inspection. When opted in, the venv/pip/import subprocesses run with a scrubbed environment (throwawayHOME,PATHonly) so a malicious candidate can't read the caller's credentials/config.--draft-prcrash. The default report dir is gitignored, so finalizing the reportgit add-ed an ignored path (rc=1) and then raised. The finalize step now skips cleanly when the report path is gitignored (its content is already in the PR body), and_create_autonomous_prstages onlyuv.lock.Example tests updated to the opt-in default; adds tests for "default run installs no candidates" and "finalize skips a gitignored report dir". Full suite 179 passed / 12 skipped; ruff + mypy strict clean.
Re-review amendments.
--draft-prnow skips any report dir git cannot commit:git check-ignoreexits 128 for a path outside the repository, which the previous guard read as "not ignored", staging a doomed path and raising — the original crash, still reachable via a custom--report-dir. Only a definitively not-ignored path (exit 1) is committed; rc=128 test added.--inspect-candidates.FixtureEvolutionRuntimeand the testRecordingRuntimenow implement the lifecycle protocol (aclose/async context manager); example code labels runtimes viaruntime_kind_value()instead of.kind.value, which raises for namespaced string kinds._isolated_envdocuments the proxy/CA trade-off of the credential scrub (candidate installs may fail behind corporate TLS interception — accepted).Round 2 (re-review follow-up)
--inspect-candidatesopt-in): all three probe subprocesses run with the shared scrubbed environment, skipped candidates are recorded explicitly, and a one-sided skip cannot read as a "breaking" behavior diff.https://github.com: a protocol-relative href (//evil.example/...) in user-generated discussion markup previously urljoined into an off-site GET.