fix(actions): Lopu repairs failed PR checks - #574
Conversation
|
These examples are warning-only. They do not fail a required check or block this PR.
Sanitized tail ' --effort low\n' +
' --max-turns 1\n' +
' --dangerously-skip-permissions\n' +
' --allowedTools ""\n' +
'\n' +
' - name: Report the live credential result\n' +
' env:\n' +
' CREDENTIAL_SLOT: ${{ steps.live_probe.outputs.claude-credential-slot }}\n' +
' CREDENTIAL_NAME: ${{ steps.live_probe.outputs.claude-credential-name }}\n' +
' run: echo "Live Claude authentication succeeded with $CREDENTIAL_SLOT ($CREDENTIAL_NAME)."\n' +
'\n' +
' route:\n' +
' if: >-\n' +
" inputs.promotion_source_pr == ''\n" +
" && inputs.promotion_plan_b64 == ''\n" +
" && (inputs.maintenance_operation == ''\n" +
" || inputs.maintenance_operation == 'manage-prs')\n" +
" && !(github.event_name == 'workflow_dispatch'\n" +
" && github.actor == 'github-actions[bot]'\n" +
" && github.ref_name == 'github-actions'\n" +
" && inputs.pr_number == ''\n" +
" && inputs.branch == 'lopu-internal-all-branch')\n" +
" && (github.event_name != 'issue_comment'\n" +
' || (github.event.issue.pull_request',
expected: /anthropic-api-key-fallback:/u,
operator: 'match',
diff: 'simple'
}
Node.js v22.23.2
Sanitized tail '# YAML is loaded from an arbitrary PR base or head ref.\n' +
'# - `ai-merge-paused` is a user-controlled, durable stop signal. Automation\n' +
'# never creates, adds, removes, or treats it as stale: when present, every\n' +
'# detector and worker abstains until a user removes it. This prevents base\n' +
'# branch movement from silently re-spending AI/Vercel/GitHub compute.\n' +
'#\n' +
"# graphify-out/** is never given to the AI. The repo's merge driver for\n" +
'# graph.json is not configured in CI, so git would silently text-merge it into\n' +
'# a mixed base+head union (the poisoned-pair state CLAUDE.md forbids). Instead,\n' +
'# when BOTH sides touched graphify-out since the merge base, the whole\n' +
'# directory is deterministically reset to the base side before anything else.\n' +
'# AFTER the resolution is verified and committed, the graph is refreshed on a\n' +
'# pristine reset tree and committed separately, so the pushed graph reflects\n' +
'# the merged code. That ordering is required: the verify step asserts the\n' +
'# staged graphify-out subtree still equals the base side, so refreshing\n' +
'# earlier would fail its own check.\n' +
'#\n' +
'# The refresh includes LLM SEMANTIC extraction when a configured Lopu provider\n' +
'# credential exists: `graphify extract` + `graphify cluster-only` through\n' +
'# OPENAI_API_KEY, ANTHROPIC_API_KEY, or CLAUDE_CODE_OAUTH_TOKEN. The configured\n' +
'# review backend is preferred, so Terra/Sol-based Lopu review and Graphify use\n' +
'# the same OpenAI project credential while Claude remains a fallback. Extract\n' +
'# is manifest-incremental, and unchanged\n' +
'# content is served from the tracked content-addressed semantic cach'... 487078 more characters,
expected: /feature_stack_merge:[\s\S]*?if: >-\s*!cancelled\(\)\s*&& needs\.feature_stack_plan\.result == 'success'\s*&& needs\.model_config\.result == 'success'/,
operator: 'match',
diff: 'simple'
}
Node.js v22.23.2
|
github-actions
Bot
added
lopu: mergeable
🤖 Lopu — review of
|
| Scope | Files | Bytes |
|---|---|---|
graphify-out/snapshots/v1 |
130 (26 trees × 5 portable) | 1,128,478,540 |
graphify-out |
996 | 1,130,880,398 |
| whole tree | 3,162 | 1,242,006,530 |
That's 91.0% of the checkout — matching the body's "996 files / 1,130,880,398 bytes / 91% of 1,184 MB" precisely.
Blast radius is exactly the portable snapshots
trackedUnder(SNAPSHOT_NAMESPACE) returns every tracked path under the namespace, not just portable ones, so I checked what's actually tracked there before trusting it. On develop it's exactly the five portable basenames, 26 of each — no graph.html or other derived artifact is tracked under snapshots/v1. The "stale non-portable tracked file gets swept into a deletion" edge case doesn't exist in this repository.
Two other things I verified rather than assumed: removed is a new key on the returned JSON, and no caller parses these fields (checked all six call sites), so the addition is safe. And both new loops batch at 100 and no-op on an empty list, so there's no empty-argv git invocation.
The fail-closed guard on snapshots.length is the right call — pruneSnapshots always activates a valid snapshot before pruning, so an empty namespace means a failed build, not a legitimate total prune.
Validation
node .github/scripts/stage-graphify-snapshots.mjs --self-test
→ Graphify snapshot staging contract: self-test OK (exit 0)
The extended self-test is a real regression test, not decoration: it makes the controller commit and then asserts the pruned tree is untracked, the active tree intact, and git status --porcelain -- graphify-out empty. The --no-renames note is a genuine subtlety — byte-identical fixture trees would otherwise pair each prune with the new snapshot and hide the deletion.
Sequencing note
Once this lands, the 25 excess trees need no separate cleanup commit. The next controller Graphify refresh prunes to retention and this stager now carries those deletions into the commit — the cleanup is emergent. That resolves the ordering concern I raised on #516 ("pruning now wouldn't hold"). #565 closes the other regrowth path (main → develop sync); with both landed, the bound holds on both branches.
Checks: 79 green, 0 failing. CodeQL: no open alerts on this head.
— Lopu (Claude Opus 5) 🌸
Lopu repository reviewLopu reviewed this PR against github-actions as Thingtime's principal PR and repository manager. Using Claude Opus 5. Lopu found no justified local change to publish from this review pass. Lopu review — PR #574 ·
|
|
🤖 Lopu — independent confirmation of this fix, from the other end I hit this same defect from scratch while reviewing #516 and reproduced it before finding this PR. Posting the corroboration rather than a second fix: I reverted my duplicate, because this version fails closed on an empty snapshot namespace and mine did not. Reproduced end-to-end, different fixture shapeThree real content-addressed snapshots committed to a fixture repo,
Same conclusion via a different route, so the diagnosis isn't fixture-shaped. The consequence worth spelling out in the PR bodyThe silent bloat is the visible half. The other half is that this breaks the promotion refresh outright. git diff --quiet && git diff --cached --quiet \
|| fail "Derived Graphify commit left tracked worktree or index changes."Unstaged prune deletions trip that guard directly. And the earlier gate — if git diff --cached --quiet; then emit refreshed nochange; exit 0; fi— means a run whose only Graphify change is a prune reports One thing I checked because this fix newly introduces
|
DEFAULT_SNAPSHOT_RETENTION |
1 |
Snapshots on main@4adda985 and develop@f31864b2 |
26 |
Tracked bytes under graphify-out/snapshots/v1 |
1 077 MiB (largest single snapshot 51 MB) |
All 26 validate as well-formed records, so nothing is corrupt — it's ~1.05 GiB of correctly-formed, superseded output that the router keeps pruning and the commit keeps re-adding.
Merge order: this before #516. Once staging can carry a prune, the next controller Graphify run collapses the store to the bound and reclaims it in one publish; merging the promotion first just carries 26 across again.
Separately, while chasing this I found that the other half of the snapshot design is also inert — 0 of 26 snapshots record a source_tree that exists in the repo, because computeSourceFingerprint() hashes the co-located ./trusted checkout as a gitlink, so the exact cache key can never hit. Different file, fixed separately; details on #516 so this PR stays single-purpose.
🤖 Posted by Lopu, Thingtime's PR manager.
|
🤖 Lopu — the fix is right, but it changes how every cross-branch Graphify merge behaves I re-reviewed What follows is one consequence I don't think has been named yet, and it's worth naming before merge. Staging prunes makes cross-branch merges conflictOnce prunes actually land, a controller Graphify commit is a delete+add pair under Same scenario, same fixture, the repo's real This is not a fixture artefact — it reproduces with fully unique per-tree files, and it reproduces at exactly the steady state this fix creates: each branch holding one snapshot (the retention bound), two branches refreshing from a common base. On the real data there's also an exact-rename floor: It also invalidates the invariant our own Where that lands
So the system self-heals, but Graphify-touching merges go from clean to always conflicting, then resolved on a round-trip. That's a real shift in merge economics — worth choosing deliberately rather than meeting in production. I deliberately did not try to fix it here. Git has no per-path rename-detection control: no Two smaller things
The body's "Follow-up (not in this change)" is stale. It says the 26-snapshot backlog survives this fix and needs a manual The first controller Graphify commit after this lands collapses the backlog itself — ~125 of 130 tracked files, ~1.0 GB, in a single commit. The "Sequencing note" above already called this correctly; this just confirms it against the real router. Only the description needs updating — but anyone using it to predict the next controller commit will be surprised by a ~1 GB deletion. 🤖 Posted by Lopu, Thingtime's PR manager. Claude Opus 5. |
|
🌸 Lopu — batch review note (conflict-batch Reviewed and clean. The The fix is right, and the scale of what it fixes is bigger than the diff
Retention is 1. This PR is one half of what stops that (the other is #565 for Two details I checked rather than assumed:
Validation: self-test passes; with One note, no action: the fail-closed guard keys on "≥1 portable file exists", which |
|
Lopu — repository review The bug you're describing is real and I measured it on
A 34× overshoot is exactly the "retention reports itself as enforced while nothing lands" failure mode, and it independently corroborates #565. Between you, the two ends of the leak are covered: this PR makes prunes reach the commit, #565 re-applies the bound to a merge result that unioned two already-bounded sets. On the implementation
The fail-closed guard is the detail that matters most. Computing
Validation I ran:
The mutation test is the one I care about: it proves the new assertions detect the original bug rather than passing alongside it. The two red contract advisories here are pre-existing on Verdict: approve. No changes needed. Worth merging alongside #565. |
🤖 Lopu review — approve; this is one of the higher-value PRs in the batchThe bug is exactly as described — staging only ever walked files that still exist, so the router's prunes never reached the commit: every controller run re-tracked the trees it had just pruned and left the removals as unstaged deletions. Retention reported itself as enforced while nothing was removed in Git. I measured what that has cost:
So #574 (prunes reach the commit) and #565 (re-apply the bound after a sync merge) are the two halves of the repository-health fix, and they're worth more than their diff size suggests. Implementation notes I checked and agree with:
Validation: |
🌸 Lopu — real bug, and the self-test asserts the property that would have caught itMerge order. The contracts run in a non-blocking advisory job, so green checks here don't certify them. Two are already red at The diagnosis matches what's visible in the repository. Staging only ever walked files that still exist, so every controller run re-tracked exactly the trees the router had just pruned and left the removals unstaged — retention reporting itself as enforced while a promotion branch accumulated one snapshot per merged branch. You can see the shape of it in this same review batch: the open product PRs each carry a fresh ~1.33M-line The things I checked rather than took on trust:
The self-test assertion I'd single out is the last one: after committing, it checks Complementary to #565, which bounds the same accumulation from the merge side (a main→develop sync unions two differently-named content-addressed trees and nothing re-applied the bound to the result). The two are independent and both are needed; neither subsumes the other. No changes made. Lopu · automated repository review · 0 open CodeQL alerts on |
|
🤖 Lopu review — ✅ validated, no changes requested. Flagging the severity of what this fixes, because it's easy to under-read. This is a silent unbounded-growth bug, not a cosmetic staging nit. Staging Three details I checked and agree with:
The self-test extension is adversarial rather than confirmatory: it builds a This is the strongest single change in the current review batch. |
🌸 Lopu — correct, and the self-test is stronger than the code it coversNo changes needed. The diagnosis is precise: Things I checked rather than assumed:
The self-test earns particular credit. Coordination note. #565 re-applies the same retention bound at the main→develop sync merge, where content-addressed trees union rather than conflict. Complementary, not overlapping: this makes prunes reach the commit at all, #565 re-bounds a merge result nothing had re-bounded. No file overlap; verified they merge cleanly together and alongside #584/#573/#577/#580/#579, all 15 control-plane contracts green on the combined tree.
|
|
Lopu repository review — batch review of 16 open PRs. This one has a visible symptom in this very repository. Staging only ever walked files that still exist, so the router's prunes never reached the commit — every controller run re-tracked exactly the trees that had just been pruned and left the removals unstaged. That matches what I see across this batch: every Three things I checked specifically:
The self-test additions are the strong part — committing the result and then asserting Worth landing together with #565, which fixes the other half — retention being undone by the Validation: No changes made. |
|
Lopu review — no defects found. This fixes a genuinely self-concealing bug and I'd land it. The failure mode is the interesting part: staging only ever walked files that still existed, so the router's prunes never reached the commit — every controller run re-tracked exactly the trees it had just pruned and left the removals as unstaged deletions. That's how a promotion branch accumulates one snapshot per merged branch while retention reports itself as enforced. A leak that reports success is worth more than the diff suggests. Two things I want to call out as correct:
Deletion scope stays correctly narrowed to Validation: The two contract failures visible in this worktree are pre-existing on the Recommended batch order for the nine Posted by Lopu, Thingtime's PR manager. |
Lopu · reading this diff, and where it sits in the controller queueTwo-dot diff artifact. GitHub's "Files changed" view shows git diff $(git merge-base github-actions HEAD)...HEADSame artifact appears on #573, #574, #575, #577, #579 and #580 — worth knowing before anyone reviews the set. On the change. Cleanest of the nine: one file, one bug, a fail-closed guard, and a test that catches the regression class. Prunes never reached the commit because staging only walked files that still exist — so every controller run re-tracked exactly the trees just pruned and left the removals unstaged, and retention reported itself as enforced while the branch grew a snapshot per merged branch. Merge order. Nine Pre-existing advisory failures. |
github-actions
Bot
removed
the
lopu: mergeable
1 participant
Lopu controller check repair
Lopu identified a failed PR check whose root cause is in the protected controller/workflow code.
Lopu control-plane fix — Graphify snapshot retention never reached the commit
File changed:
.github/scripts/stage-graphify-snapshots.mjs(protected control plane)Found while reviewing: PR #516 (
develop→mainpromotion), headf31864b2Diagnosis
graphify-cas.mjsenforces snapshot retention correctly — it prunes supersededsnapshot trees from the worktree after activating a valid replacement. The
stager then decides what reaches the controller's commit, and it only ever
walked files that still exist:
A pruned tree is not in that list, so it was never passed to
git addand thedeletion was never staged. Every controller-authored Graphify commit
re-tracked exactly what the router had just pruned and left the removals behind
as unstaged worktree deletions. Retention reported itself as enforced while the
bound could never land in Git.
This is why manual prunes "work once and evaporate", and it is the durable cause
of the accumulation now visible on both
mainanddevelop: 26 trackedportable snapshots against a documented default retention of 1, totalling
1,130,880,398 bytes across 996 files — 91% of a 1,184 MB checkout. Snapshots
are content-addressed per source fingerprint, so each merge unions both sides'
trees and nothing re-applies the bound afterwards; the branch gains roughly one
snapshot per merged branch.
Reproduction against the unfixed stager
Scratch repo, two tracked snapshot trees, one removed exactly as
pruneSnapshotsremoves it:Fix
Stage the router's prunes explicitly, scoped to the one namespace it prunes:
SNAPSHOT_NAMESPACE = "graphify-out/snapshots/v1". The semantic CAS isappend-only and deliberately never has deletions staged for it.
as deletions via
git update-index --force-remove. Plumbing is deliberate:git rmapplies worktree safety heuristics, and both porcelain forms have tonegotiate the broad
graphify-outignore rules legacy product branches stillcarry.
update-index --force-removerecords exactly the deletion of exactlythose paths.
on disk, nothing is staged. The router always activates a valid snapshot
before pruning, so an empty namespace means a failed build, not a legitimate
prune — staging it would delete the only graph the branch has.
stageGraphifySnapshots()now returnsremovedalongside its existingcounters, so controller logs show the bound being applied.
Why staging deletions is safe at every call site
All three consumers restore the complete tracked tree before Graphify runs, so a
missing tracked file is always a prune and never a partial checkout:
.github/scripts/rebase-stack/refresh-promotion-graphify.shgit reset --hard+git clean -qffdx+rm -rf graphify-out+git checkout HEAD -- graphify-out/.github/workflows/resolve-pr-conflicts.ymlgit reset --hard HEAD+git clean -qffdx -e trusted/+rm -rf graphify-out+git checkout HEAD -- graphify-out/.github/workflows/rebase-pr-stacks.ymlgit reset --hard HEAD+git clean -ffdxRemoved snapshots stay recoverable from Git history, matching the documented
invariant in
AI_ALL.md.Validation
Extended
--self-test(gated bycontrol-plane-ci.yml→ Validatecontent-addressed Graphify snapshots) to cover a superseded tree tracked at the
base commit, and asserted the full post-commit state rather than just the staged
diff: the pruned tree is untracked, the retained tree keeps all five portable
files, and
git status --porcelain -- graphify-outis empty. Added thefail-closed case as well.
The new coverage fails when the fix is reverted — replacing the
removeFromIndex(root, removed)call with a comment gives:Full
control-plane-ci.ymlverifyjob replicated locally against the modifiedtrusted checkout — every step passes:
End-to-end against the same scratch-repo scenario, now with the fix:
Follow-up (not in this change)
The existing 26-snapshot backlog on
main/developis not removed by this fix —it only stops the next one from being created. Once this lands, a single
scripts/graphify prunein the canonical environment will collapse the tree backto the bound and, for the first time, that removal will survive the following
controller commit. Running the prune before this fix lands would be undone by
the very next controller Graphify commit.
Source Lopu workflow run