Critical-app editing follow-ups: per-app leases, a real preview lifecycle test, an inert secondary prioritizer - #661
Draft
gnguralnick wants to merge 29 commits into
Draft
gnguralnick wants to merge 29 commits into
gnguralnick wants to merge 29 commits into
Conversation
…writer ChatOomPrioritizer's set_adj is now optional; None makes it accept every report while resolving, computing, and writing nothing, and skip the sweep. The secondary chat passes None, replacing _refuse_to_set_oom_score_adj, which let the whole ranking run only to drop each write. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
preview_app_test.py stubs serve_isolated_instance.py and writes the state file the real script would, so drift between the two (a renamed flag or state key) passed both suites. The new integration test boots a fixture app from its [preview] table with nothing stubbed, checks the preview serves a copy of the live data, refreshes a rebuild on the same port, and tears it down, registrations included. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The careful flow held one 'editing critical apps' lease, so an unrelated
chat edit and shell edit could not run at once. Nothing needed that: the
apply refuses to run beside another apply and rebuilds a bundle whose
sources moved under it. Each pass now leases the critical apps it changes
or previews ('editing critical app <name>'), the go-live freshness check
also covers every file the pass changes, and the lead re-checks for a
notice another pass raised before applying.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: test-plan.md's C1 secondary-contract check still said the live chat's oom_score_adj stays unchanged "after the preview's sweep interval (the refusing set_adj)", but a secondary chat's prioritizer now has no writer and runs no sweep. Fix: the check now waits after a send from the preview and names the inert prioritizer (no writer, no sweep) as the reason nothing changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: critical-app.md step 4's freshness check said the branch is mergeable only if "nothing the two bundles are built from" changed, but the command diffs only the pass's own package, the shared library, the npm files, and the branch's own files. With per-app leases a pass on the other critical app can legitimately change that app's sources, which section 1 says the apply handles by rebuilding the bundle. Fix: the sentence now names what is checked and says a change to the other critical app's sources is not staleness because the apply rebuilds that bundle from the merged tree. The command is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: plan-critical-app-editing.md's summary of references/critical-app.md still listed "the lease" and a freshness check "over both frontends, the library, and the lockfile", while the reference (and line 56 of the same plan) now describe per-app leases and a check that also covers every file the pass changes. Fix: the summary names the per-app leases and the freshness check's actual scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: update-self step 5b takes an `editing critical app <name>` lease for each critical app an update touches, but its list of critical-app trees omitted system/apps/terminal_pty/, which is `critical = true` and whose lease the careful flow takes when a terminal preview boots it. Fix: add system/apps/terminal_pty/ to the list, so an update touching only the pty checks for and takes that app's lease like the others. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: the sentence saying a None set_adj makes the prioritizer inert was spliced into the ChatOomPrioritizer class docstring mid-line, leaving one 121-character line in a paragraph that wraps at about 80. Fix: break the line before the resolve_process_started_at sentence. Wording is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ss summary Problem: harden-contention.md summarized critical-app.md step 4's freshness check as the app package, workspace_ui, and the npm files, but that check now also diffs every file the pass branch changes. Fix: add the pass branch's own files to the summary so it matches the check it points at. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: critical-app.md step 4's freshness check spoke of "your app's bundle" and diffed a single system/apps/<package>/, though a pass can hold leases on several critical apps (a workspace_ui or npm-file change always holds both the shell and the chat). Fix: say the check covers every app the pass holds a lease on and to name each one's package, and call a foreign app "another critical app". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: update-self lists the critical apps by directory (including system/apps/terminal_pty/) and takes `editing critical app <name>` per app, but never says <name> is the app.toml name; terminal_pty's is `terminal-pty`, so a lease named after the directory would not match the careful flow's anchored tk ready check. Fix: state that <name> is the app.toml name, with terminal_pty as the example. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…llet Problem: the critical-app-editing plan's testing section still said `--secondary` "refuses OOM writes", the refusing-writer shape the branch replaced with an inert prioritizer. Fix: say it keeps an inert OOM prioritizer with no writer, matching the agent_manager.py bullet and the code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: update-self 5b called its trigger list "the trees the critical bundles are built from, the same set the careful flow's freshness check names", but the list now includes the terminal and terminal-pty trees (no bundle is built from them), and the careful flow's freshness check names only the leased apps' packages plus the pass's own files. Fix: say the list is every critical app's tree plus the shared library and npm files both frontend bundles are built from. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: the careful flow's freshness check now names only the leased apps' packages (so passes on different critical apps run side by side), but the test plan's E1 only checked that an edit under the pass's own app is named. Fix: E1 also commits an edit under the shell with a chat-only pass and expects the check to stay empty. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: plan-critical-app-editing.md's careful-flow summary said go-live checks freshness over 'the bundles' sources', which reads as both frontend bundles, while critical-app.md step 4 diffs only the leased apps' packages, the shared library, the npm files, and the pass's own files. Fix: name that set, matching the reference and the plan's own summary of it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: ChatOomPrioritizer.start and reapply return immediately when set_adj is None (a secondary chat's inert prioritizer), but their docstrings still said they begin the sweep and write every chat's score. Fix: add the inert case to each docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ycle test Problem: test_preview_app_lifecycle.py recomputed the forward_port.py path that preview_app.py already holds as _FORWARD_PORT_SCRIPT, so a move of either file could leave the two copies pointing at different places. Fix: copy mod._FORWARD_PORT_SCRIPT from the loaded preview_app module. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: the plan's summary of references/critical-app.md said the go-live freshness check covers "the pass's app", while the reference and the plan's own careful-flow bullet say it covers every leased app's package. Fix: say "the leased apps' packages" so the two bullets agree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ackage Problem: harden-contention.md said critical-app.md step 4 applies the freshness check over one system/apps/<package>/, but step 4 names the package of every app the pass leases (a workspace_ui pass leases the shell and chat). Fix: say "each leased app's system/apps/<package>/". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: the test plan's C1 secondary-contract check (docs/system/blueprint/critical-app-editing/test-plan.md) said a running chat's agent's oom_score_adj stays unchanged after a send from the preview. The send is real, so the live chat sees the addressed agent's turn start and its own prioritizer legitimately re-tags that agent; checked there, the step could fail for reasons unrelated to the preview. Fix: name an agent the send did not address, and say why the addressed one may move. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: step 4 of critical-app.md built the branch's own file list with a plain `git diff --name-only`, whose default rename detection names only a rename's destination, so an edit to the rename's source on the served branch passed the check and merged into the renamed file unverified. Fix: pass --no-renames so both paths of a rename enter the pathspec. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem: test_preview_app_lifecycle.py raised its budget to 180s, 18x the suite's 10s default, as a bare literal, where every other .agents test that overrides the budget names the constant and says why. Fix: move it into _BOOT_TIMEOUT_SECONDS with a note that the shared script allows each boot 60s to pass its health check and the test boots twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This reverts commit af33ed9.
With one lease per critical app, two passes that each need the shell and the chat could each take one lease, find the other's, and both stop. A pass now takes its leases in name order and releases the ones it took when any is held by another agent.
Problem: update-self's critical-app lease step read per app (check one, take one), so an update touching workspace_ui could take chat, find system_interface held, and surface while still holding chat -- the hold-one-each state critical-app.md's all-or-none rule prevents. Fix: say update-self takes them all or none as critical-app.md does: check each, take them in name order, and release the ones taken before surfacing a held one; note it in the changelog line.
Problem: the spec's careful-flow bullet (plan-critical-app-editing.md) summarized the per-app leases but not the all-or-none rule critical-app.md now sets for taking them, and the system changelog did not mention it. Fix: add the rule to the spec's lease parenthetical and to the system changelog paragraph.
Problem: critical-app.md said a pass taking its leases all or none means two passes needing the same apps never hold one each, but the growth path still lets a pass take another app's lease while holding its own, with no guidance when that lease is held, so a mutual wait surfaced as a bare "another chat is editing" message. Fix: scope the guarantee to entry, and have a growing pass that finds the new lease held surface it along with the leases it already holds, since the holder may be waiting on one of them. The changelog entry says the same.
Problem: the careful-flow bullet in plan-critical-app-editing.md said the leases are taken all or none "so two passes never hold one each", but critical-app.md scopes that guarantee to entry: a pass that grows to another critical app takes that lease while holding its own. Fix: say the leases are taken all or none at entry, so two passes needing the same apps never hold one each, matching critical-app.md and the changelog.
Problem: the all-or-none rewrite of update-self's step 5b lease rule dropped the earlier "surface instead of proceeding", leaving only "release the ones you took before surfacing it"; with no approval gate before the apply, the text no longer said outright that the apply does not go ahead. Fix: release the leases taken and surface the held one instead of proceeding.
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.
Follow-ups to three of the review comments on #552 that were still open when it merged. One commit per item.
What changes
Critical-app edits lease per app. The careful flow held one
editing critical appslease, so an unrelated chat edit and shell edit could not run at once. Nothing needed that:update_self.py applyrefuses to run beside another apply, and its bundle-stamp check builds from the merged tree instead of installing a bundle whose sources another pass changed. A pass now takesediting critical app <name>for each critical app it changes or previews (aworkspace_uior npm-file change counts as both the shell and the chat). The go-live freshness check also covers every file the pass changes, which the single lease used to guarantee, and the lead re-checks for another pass's notice before applying, since the apply replaces its rollback point.update-selfandharden-contention.mduse the per-app names;update-self's release step also stops naming the staleediting service system_interfacelease.A real preview lifecycle test.
preview_app_test.pystubsserve_isolated_instance.pyand hand-writes the state file it would write, so drift between the two passed both suites.test_preview_app_lifecycle.pyboots a fixture app from its[preview]table through the real shared script andforward_port.py, checks it serves a copy of the live data, refreshes a rebuild on the same port, and tears it down. Renaming the state file'sinner_portin the shared script alone fails this test while the 11 stubbed tests still pass.An inert OOM prioritizer for a secondary chat.
_refuse_to_set_oom_score_adjlet a preview chat run the whole ranking (pid lookups, process start times) only to drop each write.ChatOomPrioritizer'sset_adjis now optional;Noneaccepts every report and resolves, computes, and writes nothing, and runs no sweep.The other two comments (routing
/assistthroughheal-creation, and makingsystem/scriptsimportable) need no change:heal-creationitself sends critical apps to the careful flow, andsystem/scriptsstays stdlib-only so registration and recovery work without the root venv.Testing
.agents: 785 passed, 8 skipped. Rootsystem/libs458 passed,system/services453 passed,system/test_supervisord_layout.py6 passed.system/scripts: 372 passed, 5 failed, none touched by this branch: the fouragy_shimtests are known macOS-only failures (no/proc, bash 3.2), andtest_lead_address_by_id.pyfails the same way onmainhere (Docker not running). CI is the real signal for those.update-selftoo). Review passes made 18 further follow-up commits, mostly wording that keeps the reference, spec, and test plan consistent with the per-app leases. Two change behaviour:update-selfnow countsterminal_ptyamong the critical apps it leases for, and the freshness check lists both paths of a renamed file (--no-renames).