fix(dev-fleet): isolate the sync runner and pin git's object graph - #7142
Conversation
github-actions
Bot
added
readiness: checking
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Both cross-references check out: main already runs the sync runner as a snapshot under Design-Verdict: PASS A real answer-integrity gap closed at the existing single chokepoint, consistent with the two sibling pins, with behavior-level (not presence-level) test coverage. Suggestions
[DESIGN-REVIEWED] c8ac097 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All verification done. The env pin is genuinely a sibling of two existing pins, the First-Principles-Verdict: CONCERNS The graph pin earns its place; the sandbox characterization test is a rider whose fact an existing test already entails, and two counted siblings stay unpinned. What this change shipsIntent: make Dev Fleet's sync decisions read git's real history, not a grafted substitute — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] c8ac097 |
github-actions
Bot
added
the
merge conflict
9caa6f1 to
1ad9f38
Compare
github-actions
Bot
added
readiness: checking
1ad9f38 to
bfdaf34
Compare
github-actions
Bot
added
readiness: checking
Round 4: recommending withdrawal rather than a fifth patchThis is the fourth consecutive round of blocking findings in The predicate tableThe invariant, and why it is not cheaply checkableTHE PROPERTY A SAFE SKIP REQUIRES: for every input the skipped step reads, the artifact already on disk provably corresponds to those inputs as they are NOW. Concretely: WHY EVERY FINDING IS ONE BUG, NOT FIVE: each of the three decisions compares a PROXY for that property instead of the property.
IS IT CHECKABLE CHEAPLY ENOUGH TO LEAVE THE OPTIMIZATION WORTH HAVING? No — for two independent reasons. (1) (2) The build is not a pure function of WHAT IS AVAILABLE (for a maintainer weighing a redesign, not for this PR): RecommendationWITHDRAW #7142. I changed nothing and pushed nothing; head is still bfdaf34 and the remote lease matches, so nobody else has touched the branch. THE ARITHMETIC. Benefit. A Pull+Build merges everything since the last sync, so the skip needs the whole RANGE to be backend-only, not one commit. Measured over main's last 400 commits: 30.8% touch Cost. Nine falsifiers across three predicates, all fail-DANGEROUS in the same direction — a stale SPA served behind a new backend, reported as SUCCESS — which The strongest single argument is C2, which is new this round: If a maintainer still wants the saving, the follow-up worth funding is (d): a dist cache keyed by Not landable in this shape. Recommend closing #7142 with the predicate table above in the closing comment, and opening the cache-keyed follow-up separately if the ~3-20% hit rate justifies it. I did not close, comment on, or label the PR. The three findings
Notes
Needs a maintainer ruling
I have not closed, retitled or labelled this PR — withdrawing it is a call for the maintainer, not for the loop that found the problem. |
bolichen97
changed the title
bfdaf34 to
ca41c40
Compare
|
span=c905bf2468b8 — Disposition: FIXED — the mechanism was replaced, and half the suggested remedy was taken outright. The rationale, quoted:
The diagnosis is correct and it is the decisive case, not an edge one. For For build+stage the gate is re-founded rather than removed, because the property a safe skip needs — "the served bundle was produced by a completed build from exactly the source on disk now" — is a statement about history, and a producer can record it even though no comparison can observe it. Two further holes in the same class were closed while re-founding it, both of which a narrowed comparison would have kept: writing and honouring the record now require |
Superseding the round-4 withdrawal recommendationThat comment recommended withdrawing this PR rather than patching a fourth time, on two claims. Both are answered by the rewrite that is now pushed, so the recommendation is withdrawn rather than acted on. Claim 1 — two of the three remedies were insufficient. True of the suggested remedies, and that is why none of the three was applied as written. Each finding is closed by a different mechanism instead: the Claim 2 — the safe residue is a materially different PR. It is a materially different mechanism, but the PR's purpose is unchanged and the surface is smaller, not larger: What is genuinely left is a cost/benefit judgement no reviewer raised and that the rewrite does not move either way: whether a roughly 3–20% per-sync hit rate is worth a stamp file and a runtime gate. That is a maintainer call, and it is left as one. |
github-actions
Bot
added
readiness: checking
github-actions
Bot
added
readiness: action required
66373f7 to
e6a620e
Compare
Disposition: the mechanism is confirmed and FIXED; the prescribed remedy is rebutted for the seventh time; and one branch of the class is a documented residual rather than a closed one — see the last section, which is a question for the maintainer rather than a claim. Pushed as
This is a new mechanism, not a restatement. Rounds 1–2 were forged on-disk provenance, 3 the digest framing, 4 repo-controlled git config, 5 the record-refresh ordering, 6 Fixed by the remedy's own shape, applied one level deeper. Class closed by branch, not by instance:
Measured mutation matrix (each mutation applied to an isolated copy and the suite re-run):
The new test drives its change between the post-walk read's cleanliness question and its OID question, which is where the order is actually decided — the earlier tests are pinned by the re-read and pass under either order, so they are not claimed as the order's pin. The previous head's claim that swapping the two questions failed the between-the-questions test was an artifact of that test's git helper hitting a no-op The lane's remedy — revert to an unconditional build+stage — is declined on verified code, unchanged from round 6. The suppressed step is Branch (d), stated honestly, because the previous head's bound was wrong in the direction that matters. |
Disposition: fixed, with the lane's first remedy applied verbatim. Pushed as
I confirmed the whole chain in the code rather than taking it: Why the remedy is safe, checked before applying it. The serializer that matters is the run TASK, not a second lock. What landed:
|
Disposition: accepted as sound, and deliberately deferred out of this PR.
The reading of the step list is correct — build+stage is the last step and the only consumer — and so is the observation that the module docstring's argument is about verifying the tree, which is a different question from whether anything reads it this sync. Two reasons it is not in this change:
Worth doing as its own change with its own evidence; not worth folding into the round that is clearing this one's red. |
Disposition: rebutted — the widening is real, and it is the repo's existing norm for this exact class of read rather than a new imposition.
The scope claim is accepted as stated: the var is on
An operator who grafts a Dev Fleet-managed checkout loses a graft-aware view in the dashboard's git readouts; they do not lose an operation, and nothing they do is refused. That is the whole cost, and it is stated in |
github-actions
Bot
added
readiness: checking
bolichen97
changed the title
fixed — by taking the remedy the lane asked for. The finding is legitimate exactly as written, and the maintainer has ruled the documented residual not accepted, so this round does what the lane has asked for from the start:
Why the alternative was not shipped. The obvious closure — record the Why a narrowed version was not shipped either. Recording the pre-run tree OID and letting the verdict's live read decline on a moved tree closes the committed-edit arm, but a tree that changes and changes back inside one run still yields a pair that re-derives identically on every later skip — the same non-self-correcting stale pair with a smaller mouth — and it forfeits the skip after every frontend-changing sync as well. The |
fixed — the human ratified it the other way, and the mechanism is withdrawn. The lane read the trade correctly and correctly declined to close it on the PR's behalf:
The maintainer ruling on that call was that the residual is not accepted, so |
rebutted as no longer applicable, not declined on its merits. The suggestion was well aimed at the design it was reviewing: an operator staring at a stale dashboard needed a lever shorter than restarting the app backend. It is moot at this head because the skip is gone — |
fixed — by making it the change's stated job instead of a rider. The lane's objection was about disclosure and blast radius rather than about the pin being wrong:
Both halves of that are answered. The forcing pressure is gone: the equality-pin test and the local |
github-actions
Bot
added
readiness: checking
fixed — the lane's remedy, applied through this module's own allowlist. The finding is legitimate as written and the chain is complete:
Done as an allowlist rather than a denylist, which is the same shape the repo already uses for this class ( base_env = {k: v for k, v in os.environ.items() if mod._is_safe_env_key(k)}
base_env["GIT_CONFIG_GLOBAL"] = os.devnull
base_env["GIT_CONFIG_SYSTEM"] = os.devnull
The two The fix is pinned, not just applied. The test now PLANTS the hostile condition instead of assuming its absence — Sibling audit, so this closes the class rather than the instance. Every git-spawning site reachable from the two test files this PR touches:
The |
accepted-and-deferred — the sibling is real, I verified it, and I am naming it rather than in-scoping it.
Verified, with the path corrected. The module is Why it is not in this commit, stated plainly rather than left to inference. It is outside the three-dot diff — The exact follow-up, so it is not rhetorically deferred. Add #7132 is not the right home for it and I have not filed it there: that issue is the backend-only-sync build skip this PR withdraws, and the PR body's trailer is now |
accepted-and-deferred, and the remedy is narrower than the suggestion. The concern is exactly right and it is the whole reason this PR added a characterization test rather than reaching for a sandbox mask:
What is already there, which changes what the right fix is. Why not in this commit. This PR's entire touch on the sandbox layer is a test: Deferred with the fix specified, not just noted. Extend the existing |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
The generated sync runner is spawned with `python -c`, which puts the inherited cwd at sys.path[0] ahead of the standard library. A module-style app backend runs with cwd at the gateway's source root, and on the editable install Dev Fleet exists to manage that is `<checkout>/src` -- the tree being synced. So the runner's own startup imports resolve against the checkout first, in the one process here the per-step sandbox does not cover: only the step argvs go through sandboxed_spawn_argv. A `src/shutil.py` an earlier sync landed, or that an agent wrote in the checkout between syncs, therefore runs arbitrary code unconfined. It only has to be on disk when the runner starts, so it races nothing. `-I` on the interpreter takes the directory off the path for the process's whole life. A sandbox mask cannot substitute: an app backend's `extra_hidden_dirs` is silently dropped by `wrap_argv`'s nested passthrough, which now has a characterization test so a caller is not misled by it. Separately, `_GIT_ENV_NEUTRALIZERS` gains `GIT_NO_REPLACE_OBJECTS=1`. A `refs/replace/<oid>` ref substitutes one object for another in every read, so `log`, `rev-list --count`, `merge-base` and `merge --ff-only` answer about a history no checked-out commit names -- and every git answer this module acts on is a statement about the checkout on disk. It is a correctness pin first, since `git replace` is a legitimate local operation, and `update_governance` and `auto_improvement`'s clone setup already pin it. It is an env var rather than a config pair, so GIT_CONFIG_COUNT stays at 4. The frontend-build skip this branch previously carried is withdrawn. Its provenance record was derived from disk after the run, so a `website/` commit landing between the build step completing and the derivation paired the new tree OID with the older tree's bundle, and the pair did not self-correct. Closing that needs the tree OID the build actually consumed, which is knowable only inside the runner -- after the merge step -- and every channel back to the backend is one a sync step can write, which is the assumption the design rests on refusing. So build+stage stays unconditional.
Disposition: fixed — removed in The lane is right: the |
Disposition: REBUTTED — the existing test does not entail the new one's fact, and the fact is the security-relevant half.
|
Disposition: ACCEPTED AND DEFERRED — the sibling sites are real and out of scope for this PR; the Pattern harvest note already generalizes the class, and I'm naming the deferred set here. The three current |
chenmingwei23
left a comment
chenmingwei23
left a comment
There was a problem hiding this comment.
Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.
3 participants
Problem / Motivation
Two gaps in the Dev Fleet sync's git-environment hardening, both about what git
answers, not what it executes:
A
refs/replace/<oid>ref substitutes one object for another in every gitread.
git replaceis a legitimate, purely-local operation, but with it inplace
log,rev-list --count,merge-base, andmerge --ff-onlyall answerabout a substitute object graph — a history no checked-out commit names.
Dev Fleet acts on every one of those answers as a statement about the checkout
on disk, so a grafted walk makes "behind by N commits" (and the rebase/merge
decisions built on it) simply wrong.
wrap_argv's nested passthrough silently drops a caller'sextra_hidden_dirs— an app backend already runs underKIROCREW_SANDBOX_ACTIVE, so a re-wrap returns the argv unchanged beforethe mask is consulted. Nothing was misled by this yet, but it reads at the
call site like a security control and enforces nothing, which is a footgun for
any future app-backend code that reaches for it.
Why it matters
(1) is a correctness pin first and a tamper pin second: a legitimate local graft
already produces wrong sync decisions, and a malicious one is an unaudited way to
steer them. (2) protects a future caller from building a boundary on a no-op.
What changed (motivation → approach → change)
_GIT_ENV_NEUTRALIZERSgainsGIT_NO_REPLACE_OBJECTS=1. It is pinned as anenvironment variable (same precedence as
git -c, overriding anyagent-writable repo config) so it covers every git invocation from the module
at one chokepoint. It is not one of the numbered config pairs, so
GIT_CONFIG_COUNTstays at 4 — pinned by a test, since a neutralizer added as afifth pair would silently disable
core.sshCommand.platform/update_governance.pyandauto_improvement's clone setup already pinit for the same reason.
test_sandbox_argv.pyrecords that the nestedpassthrough drops
extra_hidden_dirstoday — explicitly a characterization, nota contract, so a future caller cannot be misled and so the behavior is visible
if nested confinement ever becomes possible.
dev-fleet.md) gain a "Git environment hardening" section explainingthe two jobs the neutralizer dict now does, and a note that the generated sync
runner already carries
-I(existing main behavior) as the isolation boundary asandbox mask cannot substitute for.
Note on scope: an earlier head of this branch also added
-Ito the sync runnerand withdrew a frontend-build skip. Main has since superseded both — the sync
runner is now a digest-verified snapshot run by path under
-I, and build+stageis unconditional again — so after rebasing onto current main this PR carries only
the two additions above plus their docs. The runner-
-Igoal is fully achievedby main's snapshot design; the docs describe that existing behavior rather than
introducing it.
Tests
test_git_env_neutralizers_present— assertsGIT_NO_REPLACE_OBJECTS=1is inthe sync env and that
GIT_CONFIG_COUNTstays 4.test_the_neutralizers_answer_from_the_real_object_graph— runs real git in atmp_pathrepo with a plantedrefs/replacegraft and provesrev-list --countanswers from the real graph under the neutralizer.test_the_passthrough_silently_drops_extra_hidden_dirs— characterizes that anested
wrap_argvreturns the argv with no mask and no launcher script.All hermetic (every git runs
-C <tmp_path>with global/system config pinned toos.devnulland inheritedGIT_*stripped).Manual verification
N/A — unit coverage is sufficient; the neutralizer test exercises the real git
behavior end-to-end in a throwaway repo.
Related Issues
Refs #7132.
Pattern harvest
Rule candidate: agents-md
Pattern: a git/tool environment-hardening list that pins code execution (protocol, hooks, credential helper, sshCommand) but not which object graph / namespace the tool answers from.
GIT_NO_REPLACE_OBJECTS=1closes that gap here, matching theupdate_governanceandauto_improvementclone-setup pins. Any site that trusts a git count/ancestry answer about the on-disk checkout should pin the real object graph alongside the protocol/helper pins.Checklist