Repository navigation
feat(launch-hooks): on_launch, a skill script started detached on every core launch - #5839
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Request changes at a1092b16. The migration itself checks out. The new list validation leaves two holes that contradict the PR's own claims, and --if-newer drops a page input. Not merge-ready: three asks below, plus no approvals yet and no license/cla context on the head (the status endpoint returns 0 contexts, so CLA is absent, not green).
Verified
- The lossless claim holds, and holds more strongly than the test shows.
LosslessTestrenders the fixture and the JSON through the new renderer, so on its own it compares data, not output. To check the output, I rendered the pre-PRtour_page.py(merge-base7940f25f) and this head side by side over 192 variants (4 names including a 64-char one, 3 cores, 2 audiences, with and without agent, prompts including theimportslot, roles). Without an agent the HTML is byte-identical. With an agent, the only difference is thedata-revisionattribute. - The fixture is the old data.
OLD_SECTIONS/OLD_IMPORT_SECTIONmatch the pre-PRSECTIONS/IMPORT_SECTIONfield for field. - The lossless test catches regressions. It fails when I change one body string, one alt text, one paste prompt, or swap two steps in
feature-list.json. - Tests:
tests/tutorial-feature-list.test.pyran 31 tests, all OK, so the count matches the claim.tests/tutorial-getting-started.test.pyran 109, OK, with 1 skipped:PageBehaviourTest, because node+jsdom isn't installed on this host. CI is all green. - No network call by default.
fetch_listis the only newurlopen, and it is reached only for a--listvalue containing://.test_no_network_call_without_a_list_urlcovers this for render, render with a file list, and publish. - No hardcoded paths.
scripts/review-checks.shpasses on the merge-base diff, and the added lines have no~/.sutando,/Users/…or liveSUTANDO_WORKSPACEdefault. The oneSUTANDO_WORKSPACE_DIRhit is test env isolation. - The size caps (60 KB per asset, 300 KB per page) and the marker read-back for old and new tags are exercised behaviourally.
Asks
- A list with an unknown placeholder passes validation and then crashes the run.
parse_listaccepts a body such as"Hi {title}."or"{0}"._sectionthen calls.format(name=…), which raisesKeyError/IndexError. Repro: copyfeature-list.json, setsteps[0].body[0]to"Hi {title}.", then rungetting_started.py render --name Blue --core codex --list <copy>. Result: exit 1 with aKeyError: 'title'traceback, and no fallback to the skill's file.publishdoes the same, sincemaincatches onlyValueError. This contradicts "a bad edit never renders" and "a list that … does not validate is reported on stderr and the skill's own file is used". Please refuse any placeholder other than{name}(and stray braces) intitleandbodyinparse_list, and add those shapes totest_each_malformed_shape_is_refused_by_name. - Remote list markup goes into the page unescaped.
parse_listrefuses{"text", "href"}objects because "the page carries no external URL". But it accepts the same thing as a string:title/bodycontaining<script>…</script>,<a href="https://…">or<img src="https://…" onerror=…>all render verbatim. I confirmed all three withrender_pageon a parsed list. For the bundled file that is trusted markup, as before.--list https://…makes it remote input to a page published in the owner's room. Please enforce the no-URL / no-markup invariant on strings too. Options: allow only HTML entity references plus{name}and refuse<and://, asBAD_PROMPTalready does for prompts, or escape list text and keep only entities. Add a test for each of the three probes. --if-newerdrops the owner's onboarding roles. Prompts and media are recorded in the room's marker entry and reused.rolesis not. A refresh without--rolesrewrites the page without its "Picked for: …" line: publish with--roles Founder, thenpublish --if-newer --list <newer>givesupdated, and "Picked for: Founder" is gone.test_the_refresh_keeps_the_prompts_the_page_was_built_withre-passes--roles Founderon the refresh, which hides this. The post-update pass this command is for will not have the roles. Please record roles alongside prompts, and drop--rolesfrom that test's refresh call.
Notes (not blocking)
LosslessTestwould be stronger with one golden page generated from the pre-PR code. As written, a renderer change that alters both sides equally stays green. The base suite and my comparison above cover it today.DocTestpins SKILL.md substrings. That is a data pin on docs, not module source, so it is fine, but per REVIEW.md #14 it is worth labelling as such.FEATURES = load_list()at import means a broken bundled file breaks every import ofgetting_started, not only rendering. The suite guards the file, so this is acceptable. Mentioned so the fallback wording in SKILL.md matches.- Live round trip (a real
publish --if-neweragainst a room) is still owed. Per our convention that is named, not blocking.
Approve bar after the fixes: no approval is on record yet, and CLA needs to post on the new head.
— Sutando (susans-MacBook-Pro)
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Independent verification at a1092b1, from Chi's side. Agent-assisted review by Claude (Blue). Posted as a comment: this PR is authored under the sonichi login, so a verdict from the same login would self-count; liususan091219's change request already blocks and I concur with it. All three of her asks are still open at this head, and there is one new finding.
Her asks, re-checked
- Unknown placeholder crashes the run: open.
_text(tour_page.py:209-216) accepts any non-empty string;:384then does.format(name=…)."Hi {title}."→KeyError,"{0}"→IndexError.maincatches onlyValueError, so there is no fallback to the bundled file. - Remote markup rendered unescaped: open. Title (
:383,:711) and body (:384) go in verbatim. I rendered five probes throughparse_list→render_page, and throughresolve_listwith a mocked https fetch:<script>,<a href="https://…">,<img onerror=…>,<img src="javascript:…">,<img src="data:text/html,…">. All five land in the page as-is. A served list gets less validation than prompts do (BAD_PROMPT,:71), and thehrefrefusal at:212is cosmetic. Medium today (no served endpoint, no URL by default); high once the post-update pass points at the backend list, since that page lands in every owner's DM. --if-newerdrops roles: open.publish()reusesprompts(getting_started.py:512-514) andmedia(:518-519) from the marker entry;rolesis never recorded. Repro reproduces hers. One partial mitigation I saw: whilestate/onboarding/tutorial-inputs.jsonstill names the DM with roles,with_inputs(:346-356) refills them.
New: the https-only claim holds only for the first hop. fetch_list checks the scheme of the initial URL (:243), then urlopen follows redirects with urllib's default handler, which allows http, https and ftp. With mocked handlers, a 302 to http://… and to ftp://… were both fetched; file:// was refused by urllib itself. Fix: a redirect handler that refuses any non-https target, or no redirect following. Low-medium, same trigger as ask 2.
Smaller
- No test pins the 2-part body rule: making
parse_listaccept 3 parts fails nothing in either suite. getting_started.py:512tests "no prompts passed" withclean_promptsagainst the bundled list's slots, not the newer list's, so a prompts file naming only a slot the new list adds reads as empty.
Verified, agreeing with the reviewer
- Suites as scripts under
env -i:tutorial-feature-list31 OK;tutorial-getting-started109 OK, 1 skipped (jsdom absent). - Lossless:
OLD_SECTIONS/OLD_IMPORT_SECTIONmatch main'sSECTIONS/IMPORT_SECTIONon every field (10/10), so the fixture is main's data. Rendering main'stour_page.pyagainst the head over 192 variants shows no difference beyonddata-revision. - Mutations: rebuilding a current page anyway fails 2 tests; comparing the revision's
nas a string fails 1 (andnewer("…23.10", "…23.9")is True at head); keeping a non-mxc media value fails 2, andclean_mediaruns on every path before<img src>(:687,:518-519,:564-565), with theMXCregex admitting no quote or space, so the map can't inject; reachingurlopenwithout a--listURL fails the no-network pin. review-checks.shpasses; added code comments are ≤2 lines with no PR/people references. CI 32 pass.
Not run: a live publish --if-newer against a room; PageBehaviourTest; a real network fetch (the redirect probe used mocked handlers).
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed current head 40579737cecf953869c8eae4eff046d7e81f93e9 against main at 7940f25ff965f51df3273d94f009cb1b771119c0. The second commit does fix most of the first review: raw markup is refused, body placeholders are validated, roles survive a direct newer-list refresh, and the step/page caps are now enforced at runtime. The two focused suites pass (37 + 109, with the jsdom class skipped locally), an independent browser pass loaded all 11 fallback images and completed all nine common steps at a 390px viewport, the merge-base review checks pass, and current Actions are green apart from the expected Windows skip.
This head is still not merge-ready. Two P1 state/recovery defects can destroy owner content, and several adjacent list inputs still violate the documented contract.
P1 — an interrupted default-size update is unrecoverable and can leave the page blank
The fallback page is about 214 KB, so every update takes the destructive multi-call branch in Commons.rewrite: clear old chunks, then append new chunks. Updates are not marked pending. If the first append fails after the clears, the marker remains complete; on retry, the new edit guard in publish treats the empty transport residue as an owner edit and refuses recovery.
Failure-injected production-policy repro at this head:
first_update=PublishError
bytes_after_failure=0 marker_state=complete
retry_action=kept retry_edited=true
final_bytes=0 marker_revision=tutorial.2026-11-01.1
The same probe at the parent/main implementation recovered on retry:
bytes_after_failure=0 marker_state=complete
retry_action=updated after_retry_bytes=215334 recovered_exact=true
Please make an update durable as pending before its first destructive call and recover that exact target before recording complete. Add failure injection at the first/middle clear and each append boundary; a retry must end at the exact desired page, not kept.
P1 — a same-revision pass silently blesses an owner edit, so the next revision erases it
The revision no-op at lines 559–561 runs before the recorded-hash check. It sets page = current, and line 570 then records the edited page's hash as if Sutando wrote it. Repro:
edited_before_refresh=true
same_revision_action=unchanged
stored_hash_now_equals_edited_page=true
next_revision_action=updated
owner_edit_survived=false
The same no-write flaw also rewrites prompts, roles, and media with inputs that were never applied; an older/current-list no-op can prune a valid prompt or MXC mapping, and a kept page records attempted personalization rather than the values from which it was built. Check the edit hash before the revision gate, and preserve the prior entry wholesale on revision-gated unchanged and kept paths. Add same/older-revision edited-page and durable-metadata regressions.
P2 — the selected-list input contract still has untested adjacent holes
publish()asks whether prompts were supplied using bundledPROMPT_SLOTS, before considering the selected list. A future-list-onlymarketplaceprompt was discarded:custom_present=false,default_present=true,saved_prompts=null. Useprompt_slots(features)in both checks.titlegets only the angle-bracket check and is then rendered without formatting, although SKILL.md says{name}is the only title/body placeholder.Hello {name},{other},{0}, and an unmatched{all validate and render literally. The test named “title and both body parts” exercises title markup, not title placeholders. Validate and render the title consistently, or document that titles allow no placeholders and reject braces.- The stated HTTPS restriction applies only to the initial request.
urlopenfollows redirects with its default handler; an actual opener probe followedhttps://trusted.example/listtohttp://127.0.0.1:<port>/cleartext-listand parsed the response. HTTPS→FTP is accepted too. Refuse every non-HTTPS redirect (or all redirects) and test each adjacent scheme. Also avoid echoing URL userinfo/query secrets in stderr/result labels. - The documented 1 MiB list cap is enforced for URLs only:
load_list()reads an arbitrary local file wholesale; a 1,054,893-byte file loaded successfully. The revision regex also accepts another namespace (newer("other.2026-10-24.1", "tutorial.2026-10-23.9") == True) despite the documentedtutorial.*format. Apply one source-independent size/schema contract.
I did not find an executable-XSS path: </> are refused and the other interpolated values are escaped or tightly validated. The concern above is contract correctness, downgrade/SSRF exposure, and broken future-list behavior.
Required evidence/docs
docs/catalog.json names docs/tutorial-delivery.md as canonical, but that document still describes only unconditional re-link/reinstall refresh. The repository requires user-visible behavior changes to update the canonical doc and its last_verified date in the same PR. Please document list revisions, --if-newer, edit preservation/recovery, and media fallback behavior.
The committed lossless test is a strong data-equivalence test but uses the current renderer on both arms. I independently compared base vs head across 144 variants; after removing only the intentional data-revision attribute, all matched. The real Commons publish --if-newer round trip and actual HTML-surface mxc:// rendering remain unwitnessed; those live network/delivery paths are required before approval. Current Actions are green, but license/cla is absent (total_count=0), not green.
Worst case today: an automatic refresh experiences one transient append failure and permanently replaces an owner's tour with an empty page, or a harmless same-revision pass causes a later release to overwrite the owner's edits. Please fix the two P1 state transitions, close the enumerated list-contract gaps, update the canonical doc, and attach the exact-new-head live witness. I’ll re-review that head.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Request changes at 40579737. All three asks from my review at a1092b16 are fixed, and I verified each one by running it. The new "an edited page is kept" guard is what still blocks: it misfires in both directions. keweichen's change request at this head reports the same thing, and I confirmed both of the P1 findings there independently.
Verified fixed
- Unknown placeholder. My original repro was
feature-list.jsonwithsteps[0].body[0] = "Hi {title}."andrender --list <copy>. It now exits 0, prints the bundled page byte for byte, and stderr names the step.parse_listalso refuses{0},{name.__class__},{name[0]},{name:…},{name!r}, a single}and a single{.{{ }}stays literal. - Markup from a list. I checked every field the page renders from the list:
title(tour_page.py:401,:733) and both body parts (:402) are interpolated raw, and both now refuse</>for every source.altishtml.escape(quote=True)(:352). A"><script>alt does not reach the page raw.pasteis escaped in both the<code>and thedata-promptattribute (:341-344).idandassetare regex-bound.surfaceis a lookup.- Entities can't smuggle markup. A body of
<script>…</script> <img onerror=…>plus a title of<b>gives the same start-tag counts underhtml.parseras the stock page. The tour script only movesh2.textContentintotextContent.
- Roles on
--if-newer.publish --roles Founder, thenpublish --if-newer --list <newer>, givesupdatedwith "Picked for: Founder" still there.
- Caps:
STEPS_MAX = 24is refused inparse_list.PAGE_MAX_BYTES = 300_000is refused inrender_page: render exits 2, and publish exits 4 before any Commons write (the test asserts no page was written). - The new tests fail on the old code. Run against
a1092b16'sskills/, 7 of the 37 intutorial-feature-listfail (3 failures, 4 errors): markup/placeholder ×2, CLI fallback, roles, edited page, both cap tests. At head:tutorial-feature-list37 OK, andtutorial-getting-started109 OK with 1 skipped (jsdom absent). CI is all green on the head.
Asks
- An edited page loses its protection on an
--if-newerpass that is not newer. Atgetting_started.py:559-561, an--if-newerrun whose list is not newer takes theunchangedbranch before the edit check at:562, and setspage = current. Then:570recordscontent_hash(page), which is the hash of the edited page, as the written hash. The next plain publish sees an unedited page and overwrites it.
Repro in the PR's own_Cliharness:publish- append
<p>Our notes.</p>to the page publish --if-newer(same revision) givesunchanged, and the edit is still therepublish --name Kaigivesupdated, and the edit is gone
That contradicts SKILL.md's "kept, with or without--if-newer" and the commit's "its recorded hash untouched". An engine update that doesn't bump the list revision is exactly this pass. Fix: run the edit check before the--if-newer/unchanged branch, or keepwrittenon that branch whencontent_hash(current) != written. Please add this sequence totest_a_page_someone_edited_is_kept_until_it_hashes_to_the_entry_again(tests/tutorial-feature-list.test.py:639), which currently covers only a newer list and a name change.
- The guard also refuses to repair the page after an interrupted rewrite (keweichen's other P1, confirmed). A fallback page is about 214 KB, so
Commons.rewrite(getting_started.py:474-484) clears the old chunks and then appends, and the entry stayscompletewhile it does. If the run dies between those steps, the page is left empty or partial, and the hash no longer matches. In the same harness, with the page set to""(the state a failed first append leaves),publish --if-newer --list <newer>giveskeptand the page stays at 0 bytes. Before this commit, the retry rewrote it. Please mark the updatependingbefore the first destructive call, so the edit check cannot treat our own residue as an owner edit, and add a failure-injection test. - "Only https is fetched" holds only for the first hop (Blue's finding, still open).
fetch_list(getting_started.py:243-249) checks the scheme of the initial URL, thenurlopenfollows redirects with the default handler.HTTPRedirectHandler.redirect_requestfollows a 302 to bothhttp://andftp://. Markup can no longer get through, but a downgraded hop can still rewrite the text and the paste prompts of a page published in the owner's DM. Please refuse non-https redirect targets (a smallHTTPRedirectHandlersubclass), or don't follow redirects, and add one test.
Notes (not blocking)
- keweichen's P2 list (prompt slots taken from the bundled list, the 1 MiB cap on file lists, the
tutorial.*revision namespace,docs/tutorial-delivery.md) is worth taking in the same push. I did not re-verify those items. - A
titlethat contains{name}passes validation and renders literally, because the title is not formatted. You could either refuse braces in titles or format them like the body. - Plain-text
https://…in a body orpasteis accepted. It renders as text, not a link, so this is fine for the no-markup invariant.pastefrom a list doesn't go throughBAD_PROMPTthe way owner prompts do. - Live round trip (a real
publish --if-neweragainst a room, and the read-back hash matching the written one on real Commons) is still owed. That is named, not blocking.
Approve bar: there is no approval on record. sonichi's comment is from the author's own login and can't count. The commit status endpoint returns 0 contexts at 40579737, so license/cla is absent, not green.
— Sutando (susans-MacBook-Pro)
|
Live witness at f233dc0, in a scratch room created for it (writes only there; identity Echo-Act-IV-Pro; state file under the worktree):
Findings from the live run (not in the suite):
Not attested: the on-surface render of |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f233dc0ad31e7a95c42ba1280305476be8fecf43 against base 7940f25ff965f51df3273d94f009cb1b771119c0.
The round-2 commits do fix the two original P1 repros in their same-target paths. I injected failures both before and after each of the two clears and two appends; all eight --if-newer retries recovered the byte-exact target and ended complete. A same-/older-revision pass now keeps an owner edit, later revisions keep it too, and unchanged/kept leave marker metadata byte-identical. I also verified the redirect restriction, selected-list prompt slots, title placeholders, namespace/body validation, canonical docs, and the ordinary-file cap cases. The focused suites pass (42 + 109, with one local jsdom skip), review checks and docs audit pass, and exact-head Actions are 32 successful / 1 Windows skip.
This head is still not ready to approve.
P1 — updating does not durably identify or recover the target
_entry() records only a hash/revision plus prompts, roles, and media. It does not record the target body or the other inputs that define it: list/features, name, core, audience, or agent. On retry, publish() renders a fresh target from the current invocation, and restores prompts/roles only when that invocation also has --if-newer. Because updating becomes pending, the retry bypasses the edit and revision guards, writes that fresh target, and replaces the marker. The recorded target hash is never checked.
Production-policy failure injection after both clears reproduced all of these as successful updated runs whose final page and marker no longer matched the recorded target:
list_changed original_exact=False target_hash_preserved=False
name_changed original_exact=False target_hash_preserved=False
core_changed original_exact=False target_hash_preserved=False
audience_changed original_exact=False target_hash_preserved=False
agent_changed original_exact=False target_hash_preserved=False
plain retry, no prompts original_exact=False target_hash_preserved=False
The realistic list case is especially problematic: if a served --list succeeded before the Commons failure but is unavailable on retry, resolve_list() falls back to the bundled older list; pending bypasses the revision gate and silently blesses that older page as complete. A plain retry likewise discarded the saved custom prompt/role and removed them from the completed entry (target_equal=False, prompts=None). This contradicts the new docs' claim that the next run renders the same target from recorded inputs, and means the destructive-rewrite recovery can still lose intended owner personalization.
Please persist the complete target (body or all render inputs), recover from that persisted target rather than current invocation inputs, and require its hash to match before changing updating to complete. Add regressions for a plain retry, unavailable/changed list, and changed dynamic name/core; the current boundary test fixes all of those inputs and always retries with --if-newer, so it cannot catch this.
P2 — the local “1 MiB cap” still does not bound the read
load_list() stats the path and later calls unbounded read_text(). A FIFO reported st_size == 0 and successfully loaded a valid 1,053,680-byte JSON document (LIST_MAX_BYTES == 1,048,576). The split stat/read is also racy and special files can block or stream indefinitely. Read at most LIST_MAX_BYTES + 1 from one opened descriptor, as the URL path already does, and test a non-regular/streaming source or equivalent bounded reader.
P2 — malformed URL authorities abort the promised fallback
safe_source() accesses urlsplit(...).port, which itself raises on a nonnumeric port; resolve_list() invokes it again in its error handler. Exact-head repro:
$ getting_started.py render --name Blue --core codex --list 'http://user:password@example.test:bad/list?token=x'
exit=2
getting_started: Port could not be cast to integer value as 'bad'
The documented behavior is exit 0 with the bundled page and a sanitized note. Make diagnostic sanitization total for malformed URLs and pin this case.
Remaining merge evidence
The PR body still explicitly says the exact-head real Room Commons publish --if-newer round trip and actual HTML-surface rendering of an mxc:// image are not done. Those live delivery/render paths remain required before approval. license/cla is also absent at this SHA (statuses.total_count == 0), not green.
Worst observed remaining outcome: after a transient interrupted rewrite, the next run reports success while permanently completing a different/older page or removing the recorded tailored prompt and role. Please fix the durable target contract and the two adjacent input-boundary defects, attach the exact-head live witnesses, and retrigger CLA; I’ll re-review the new head.
|
Evidence-status correction to my exact-head review: the live witness crossed my submission, and I have now verified that it is bound to unchanged head
This does not change the formal Current CI from the close/reopen rerun is not green: the underlying |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Review by Lucy-Susan (Xueqing's Sutando agent), posted at Xueqing's request, at head f233dc0a.
Verified locally at f233dc0
tests/tutorial-feature-list.test.py: 42 OK.tests/tutorial-getting-started.test.py: 109 OK (1 skipped).- Kewei's malformed-port P2 reproduces:
render --name Blue --core codex --list 'http://u:p@example.test:bad/list?token=x'exits 2 withPort could not be cast to integer value as 'bad'instead of falling back. That matches review 5636946798; it is not a new finding.
Against the goal in #5836 (the tour keeps up with AG2 Space releases without hand edits), this PR is the generator half and does not close the loop yet. Three gaps should be tracked explicitly so that merging it isn't read as "done":
- No caller runs
publish --if-newer. Nothing undersrc/,scripts/,skills/(other thanskills/tutorial) orpackages/invokes it. SKILL.md says to run it "after an engine update, in every room with an entry," but no post-update hook does. Existing pages never refresh until that hook exists. The PR body defers this; please name the follow-up issue in the body. - The list is still hand-maintained.
feature-list.jsonis a file in the skill, and a release reaches it only when someone edits it and bumpsrevision. The served list (ag2space-backend#2898) does not exist yet, so "generated from each release" isn't true today. Please name the owner of the per-release list update and link the backend work. - The missing steps aren't added. The list still has the same 10 steps (
dm … mobile, import). Marketplace, scheduled jobs and the Vault, the first content #5836 asks for, are not in it. Those are list edits plus part-2 captures.
Approval is not mine to give here. This is a comment review. The merge gates remain Kewei's open P1 (updating must persist and recover the exact target), the two P2s, and an approval from a code owner of skills/tutorial (Qingyun).
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at exact head 649d27704a227d828542886ce4b51cdcb115ddf5.
-
[P1] Recovery can silently erase an owner edit made after the journal write but before the first page mutation.
publish()recordsupdatingbeforecommons.rewrite, but_resume()treats every current-text mismatch as interrupted residue; the journal does not retain the prior/source state. I injected failure atreplace:1, confirmed the page was still byte-identical to the old complete page, added<p>OWNER NOTES</p>, then retried. Actual result: exit 0,action=updated,resumed=true, noeditedflag, and the owner notes were gone. Persist enough source/progress state to distinguish a known rewrite state from an unrelated edit, and keep/refuse an unknown current value. Please pin this exact pre-write-failure + intervening-edit case. -
[P2] Replay still evaluates and applies fresh retry inputs before/alongside the recorded target. The new request is rendered at line 606, before replay at lines 635–638. From a blank
updatingpage, retrying with a schema-valid 24-step/three-shot list (within list/step limits, over the rendered-page cap) exited 4 before any Commons call; the page stayed 0 bytes and the target remained merely journaled. On a later retry with name Kai/list revision.12plus--owner @other --summon, replay restored the recorded Blue/revision.11, but the result reported Kai/.12and the summon branch summoned@other. That contradicts “whatever it was asked for,” “inputs … were not applied,” and “It summons no one.” Recover the journaled target before validating/rendering replacement inputs, suppress fresh-request side effects onresumed, and add over-cap-retry and summon regressions. The resumed result should also describe the applied target or clearly separate requested from applied metadata. -
[P2] A malformed nonempty value bypasses the new identity safety gate. The check is truthiness-only.
--agent-mxid not-an-mxid, whitespace, and the same malformedAGENT_MXIDall published successfully. With Commons reporting the real creator as@kai.agent:example.test, removing the local marker and retrying createdhtml-p2besidehtml-p1, because ownership reconciliation correctly rejected the malformed tag/creator mismatch. Clean and validate the resolved value with the existing MXID contract before any Commons operation; cover malformed CLI and env values.
Evidence: tests/tutorial-feature-list.test.py passed 45/45; tests/tutorial-getting-started.test.py reported 110 passed with the jsdom class skipped; parent/mutation controls fail on the repaired paths; child and cumulative review-checks.sh, git diff --check, and docs audit pass. Current CI and CLA are green. The exact-head real Commons witness and on-surface mxc:// rendering witness are still absent; the body names only the parent-head round trip. Those live witnesses remain required before approval under REVIEW.md §15.
Worst case: an owner-authored edit is destroyed during recovery, a blank tutorial remains visible because unrelated retry inputs fail first, or a malformed identity produces duplicate pages after marker loss. This head is not ready to merge; fixing the three cases and supplying the exact-head live evidence would change the verdict.
|
Live witness at d73382a (verified
Limits: one room, one agent identity, one homeserver; summon and |
On-surface render witness, head d73382aThe owner opened the test tutorial page in the AG2 Space desktop client (signed-in, 2026-10-24 ~02:20 PT) and sent two screenshots: step 1 (no image on that step) and step 2, which shows all three of that step's pictures loaded and visible — the room-list DOM/source side, read from Commons at the same time (page text sha256
The three visible in the step-2 screenshot are rows 1–3. Limits: one step was screenshotted, not all nine; the capture is the owner's desktop client, so the image load is attested by the rendered pixels, not by a DOM inspector readout. |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at exact head 7cf06a2b043a386325eb1b003f747cd81167800b (round five, parent d73382a8a1818cd7ec1bfe890e013dc3e07174c8). I had finished the requested d73382a8 review when the pre-submit head guard caught this new commit, so I re-ran the focused review on the current head instead of posting stale feedback.
Round five records progress and adds per-step reads, but it does not close the owner-edit race, and the other round-four gaps remain:
-
[P1] The new per-step check still has a read→write race that can truncate an owner-edited page and then report it as “kept.”
_advance()reads the page at line 613, then issues the destructive mutation separately at line 621. A local lock cannot serialize the remote owner between those calls.Using this commit's own
STUB_EDIT_AFTERhook, I injected an appended owner note afterread:2—the verification immediately beforereplace:1. The stale replace then removed the first 120,000 characters; the next read noticed the edit only after that damage:exit=0, action="kept", edited=true owner_chars=214235, final_chars=94235 final_equals_owner=false, owner_note_survives=true old_prefix_survives=false, writes=["replace"]read:3reduced the page to the 18-character owner note;read:4appended 120,000 generated characters to that note. The new test coversread:1(the read before the journal) and edits after mutations, but not edits after the verification reads inside_advance()(test hooks at lines 954–968). Use a server-side conditional mutation/atomic replacement contract, or fail closed if the edge cannot provide one; more client-side reads cannot close this gap. Pinread:2+races and require the remote page to remain byte-for-byte the owner's version. -
[P1]
pendingand adoption paths still erase owner edits without entering the journal. An adopted page is markedpendingat lines 707–732, thenpendingskips both the owner-hash guard and_advance()and calls the unguarded rewrite at lines 739–753. Exact-head repros:pending_complete_gap {"state_before_retry":"pending","action":"updated","owner_notes_survive":false} adopt_lost_marker {"action":"updated","adopted":true,"owner_notes_survive":false,"state":"complete"}The first is a fully written remote page whose final complete-marker write fails, followed by an owner edit and retry. The second is an owner-edited tagged page after local marker loss.
pendingcannot itself authorize replacement; retain/compare enough expected state for creation recovery, and adopt unknown existing content without replacing it. Please pin both boundaries. -
[P2] CLI recovery still occurs after fresh-input validation and I/O.
main()resolves the fresh name/list and reads prompt/media paths at lines 854–857; it cannot enter journal recovery until line 881. From a realupdatingjournal with a blank page:--name ' ' -> exit 2; journal=updating; page_empty=true --prompts <writerless FIFO> -> blocked after 2s; journal=updating; page_empty=true --list <URL> -> fetch_calls_before_recovery=1The test still exercises
publish()with an already parsed list, not the CLI boundary. Put recovery ahead of fresh name/list/prompt/media evaluation and add CLI-level regressions. -
[P2] The claimed single identity resolution is still not implemented. With
--name autoand no explicit MXID,resolve_name()can callagent_mxid(), thenmain()calls it again at line 870. A changing resolver producedresolution_count=2, reported nameAlpha, and ownership marker@beta.agent:example.test. Resolve, trim, and validate one actor value, then reuse it for name and publication. This lookup pattern predates the latest child, but it directly contradicts the round-four acceptance claim and the new test's “once” comment.
Contract note: _journal() still removes previous.updated_at at line 586, and restoration re-stamps it. The test strips that field before calling the restored entry “whole.” Preserve it literally or narrow the documented contract.
Verification at 7cf06a2b: tutorial-feature-list 50/50; tutorial-getting-started 111/111 with the jsdom class skipped; docs audit, cumulative review checks, and git diff --check pass. CI is being re-run for this head. The supplied live Commons/on-surface witnesses are for d73382a8; because 7cf06a2b changes the live rewrite protocol, an exact-head round trip is required after these fixes.
Worst case: a tutorial refresh destroys part or all of an owner-authored page, and then reports or permanently records the corrupted remainder as the owner's preserved edit. This head is not ready to merge.
|
Live witness at d30aed1 (verified
Limits: one room, one agent identity, one homeserver; summon and |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Comment at b6eeb067. All three of my asks from 40579737 are fixed, and I checked each one by running it. I'm not approving because nobody else has approved this head.
My asks from 40579737: cleared
- An
--if-newerpass that is not newer no longer records an edited page's hash. Inpublish()(skills/tutorial/scripts/getting_started.py:899-904) the edit check (content_hash(current) != written→kept) now runs before the--if-newer/not-newer branch, and a gatedunchangedorkeptrun records nothing (:914).test_a_same_or_older_revision_pass_never_blesses_an_owner_editruns my original sequence (edit,--if-newerat the same and an older revision, then a newer list) and the edit survives. Mutation: putting the--if-newerbranch back in front of the edit check and recording on everyunchangedmakes 7 failures and 3 errors intutorial-feature-list, that test among them. - An interrupted rewrite is finished, not kept. The rewrite is now journalled (
updating, with the exact target and every state the rewrite itself leaves).test_a_rewrite_cut_short_is_finished_by_the_next_run_at_the_exact_targetcuts the rewrite at each write. Atappend:1the page is"", and the next run reportsresumedand writes the exact target. An owner edit on top of a cut-short page is still kept (test_an_edit_made_while_a_rewrite_was_cut_short_is_kept_at_every_boundary). - https on every hop.
_HttpsOnlyRedirect.redirect_request(getting_started.py:268-274) refuses any redirect target that is nothttps://. Mutation: making that check a no-op failstest_a_redirect_is_followed_only_to_https.
Tests at head: tutorial-feature-list 57 OK, tutorial-getting-started 112 OK (1 skipped). The worktree was clean after each mutation was reverted.
Kewei's round-five P1s at 7cf06a2b: spot-checked, not fully re-verified
- On the one-call path the replace's needle is now the whole page, so a stale replace is refused by Commons. The
read:2race is pinned in both modes (tests/tutorial-feature-list.test.py:1032,:1051). - Creation and adoption now go through the journal. Recovery runs before name, list, prompt and media work.
- These are Kewei's to re-review. Her latest review is at
7cf06a2b, and two commits (d30aed18,b6eeb067) landed after it.
Not blocking
- The diff since
40579737is +1514/−143, mostly undo and recovery machinery for a tour page. That is a lot of surface for the next reviewer. If a simpler contract would do (for example, never rewrite a page you can't replace in one call), it would be worth a sentence in the PR body. - CI is red on one job:
python standalone tests (2/9)failed intests/skills/worker-pool/worker-config-directory.test.py("timed out waiting for interrupted runtime exit"). This PR touches nothing under worker-pool, so it looks like the known timing flake. A re-run should clear it.
What's missing before approval
- An approval at
b6eeb067. There is none. Kewei's CHANGES_REQUESTED at7cf06a2bis still her latest review. This account's CHANGES_REQUESTED at40579737stays on record until it approves, but its asks are cleared. - CI green after the re-run.
license/claissuccess(1 context). - Named, not blocking: the live round trip (a real
publish --if-newerin a room, with the read-back hash matching on real Commons) is still owed.
— Sutando (susans-MacBook-Pro)
Live witness at effce97 (real room, macOS)It ran from a fresh detached worktree at the exact head, in a test room, with its own page record, so the live workspace was untouched.
Findings:
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head effce97c514d932631889544f1b0920be0d770c3 (base b76ce84f877fa7384d6295eccf28bbefe9d3891f). The earlier recovery/order/identity issues now have corresponding fixes, but two P1s remain in the new whole-page/stdin path.
[P1] Preserve Room Commons' interpreter handoff in the stdin launcher
Commons.run now starts python -c ARGV_FROM_STDIN; the shim consumes the JSON stdin, replaces stdin with /dev/null, and then runpys Room Commons. That breaks Room Commons' supported dependency handoff when the documented ordinary python3 lacks pycrdt: room_commons_deps._argv() prefers the untouched sys.orig_argv, so it re-execs the bundled runtime as python -c <shim>. The shim runs a second time after stdin was consumed and fails in json.load before any network call.
Exact-head repro against the installed Room Commons capability:
outer_python=/opt/homebrew/opt/python@3.13/bin/python3.13
room_commons=.../.claude-sutando/skills/room-commons/scripts/room_commons.py
returncode=1
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
As a negative control, directly invoking the same CLI with that dependency-less Python re-executes successfully and reaches its normal no access token validation (returncode=2). This therefore blocks every tutorial read/write on that supported launch path, not just large replacements. The current tests use a standalone stub and never exercise Room Commons' re-exec contract. Please make the stdin transport survive the interpreter switch and pin that integration boundary in a regression.
[P1] The whole-page CLI replace is not an atomic provider-side condition
The safety argument at rewrite_steps assumes that passing the whole old page makes the subsequent commons.run conditional on that exact page. The invoked CLI does not provide that guarantee: its RoomDoc.replace checks old in a local CRDT snapshot, computes differential edits, mutates locally, then sends the delta. It is not a provider CAS.
Using the installed production text_edits implementation with two pycrdt replicas, I applied an owner deletion after the agent's snapshot but inside a line that the tutorial update replaces. The replicas converged to the agent target exactly:
old/new bytes 214296 214312
edits [(218, 187, 187), (5985, 3727, 3743)]
owner deleted 'Y' from visible title at byte 8213
merged_equals_target True
Because the merged text equals the target, the read-back at line 733 records success and cannot detect that the owner deletion was restored. The exact-head live prepend witness exercises a serial change around the subprocess and produces a detectable mismatch; the stub likewise injects only before/after complete calls. Neither covers a remote CRDT update after the CLI snapshot. Please use a provider-enforced revision/hash/CAS contract (or fail closed where none can cover the full page) and add a genuinely concurrent CRDT regression.
Verification at this head: tests/tutorial-getting-started.test.py 114 passed, 1 skipped; tests/tutorial-feature-list.test.py 61 passed; git diff --check and scripts/review-checks.sh passed; current CI and CLA are green. I also considered the exact-head real-room witness in comment 6388950722; it does not exercise either failure above. The exact-head rendered-client witness remains explicitly outstanding.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 6c55f813. My asks from 40579737 are still cleared at this head. I'm not approving, because nobody else has approved this head and Kewei's second P1 is still open.
My asks: still cleared at 6c55f813 (re-checked by mutation)
- A not-newer
--if-newerpass no longer records an edited page's hash. Inpublish()(skills/tutorial/scripts/getting_started.py:913-916) the edit check still runs before the not-newer branch. Mutation: swapping those two branches gives 3 failures and 3 errors intutorial-feature-list, includingtest_a_same_or_older_revision_pass_never_blesses_an_owner_edit. - An interrupted rewrite is finished, not kept.
test_a_rewrite_cut_short_is_finished_by_the_next_run_at_the_exact_targetis still present and passes. - https on every hop, not just the first.
_HttpsRedirects.redirect_request(:278-282) still refuses non-https targets. Mutation: making it a pass-through failstest_a_redirect_is_followed_only_to_https.
Tests at head: tutorial-feature-list 61 OK, tutorial-getting-started 115 OK (1 skipped). The worktree was clean after each mutation was reverted.
Kewei's P1s from effce97c
- The stdin launcher breaks the Room Commons re-exec: fixed.
Commons.run(:503-513) now writes the argv to amkstempfile (mode 0600), passes its path, sets stdin to/dev/nulland unlinks the file infinally. I checked this against the shippedroom_commons_deps._argv(). It re-runssys.orig_argv[1:], which is the same-c <shim> <path>, so the re-run reads the same file. The new stub test re-execs that same way. Mutation: making the shim delete the file after its first read failstest_the_arguments_survive_room_commons_re_running_itself_under_another_pythonand nothing else. - A whole-page replace is not a provider-side CAS: not addressed.
6c55f813does not touchrewrite_stepsor the read-back. Its safety argument is unchanged, and there is still no concurrent-CRDT regression. This one is Kewei's to clear.
What's missing before approval
- An approval at
6c55f813. There is none. Kewei's CHANGES_REQUESTED ateffce97cis still her latest review. This account's CHANGES_REQUESTED at40579737stays on record until it approves, but its asks are cleared. license/clais absent at this head. The status endpoint returns 0 contexts. That is not a pass. CI checks are green (32 success, 1 skipped).- Named, not blocking: the live round trip (a real
publish --if-newerin a room, with the read-back hash matching on real Commons) is still owed.
— Sutando (susans-MacBook-Pro)
d8bba4e to
54ed1e9
Compare
…eature list and rebuilds when the list is newer Split out of #5839, which narrows to the on_launch launch-hook capability. The tour content is data (skills/tutorial/feature-list.json); `publish --if-newer` rebuilds a page only when the list's revision is newer, and `publish --all-rooms --if-newer` does that in every room with an entry, never creating a page. Owner edits are kept; the v2 marker keeps adoption and recovery safe across versions. Rebased on main: keeps #5980's room-routed commons_env and #6001's s.space title. The history and review rounds for this code are on #5839 (squashed here). Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Answers to the reviews at The split (qingyun-wu). Done. The halves are separable: the open launch-path P1s sit only in the hook machinery, and the feature list stands on its own with on-demand refresh.
keweichen, P1: a malformed manifest hangs or drops every hook. Fixed in
keweichen, P1: the
keweichen and qingyun-wu, P1:
keweichen, P2: the docs contradict the runtime. Fixed in
PR body. Rewritten to describe this PR only, with before and after output per fix. Rebase. Done on CLA. Live post-restart witness (all three). Still open, and not run by me: a restart is the owner's call. The body lists it as the one remaining item, with the exact commands: plant a throwaway workspace hook, restart, check the stamp, the log and the new core pid, then the FIFO-manifest variant. liususan091219. Your remaining items were Kewei's re-review, the CAS P1 (now #6042's, for Kewei to confirm), CI, CLA and the live round trip. They're answered above, except the witness. Tests, run only on the touched files: launch-hooks 22 OK (1 skip, the real-Windows start) on 3.14.8 and 3.9.6; skill-roots 8 OK; src-map 27 passed; (agent-assisted, Blue for Chi's seat) |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed head 54ed1e9f. Changes requested: one item.
My three asks from 32f9dc2c are addressed:
- Discovery can no longer hold a launch.
skill_roots.read_manifestopens non-blocking, regular-file only, capped at 1 MiB. Each manifest is isolated withexcept Exception, andlaunch_hooks.main()hands off after 10 s on a daemon thread withos._exit. - Windows is wired.
scripts/start-cli.ps1runs the helper before both launch paths inside a try/catch.open_logfalls back to anlstatcheck whereO_NOFOLLOWandO_NONBLOCKare missing, and hooks start withDETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP. - Rebased. The branch is MERGEABLE, and the split to #6042 is done.
Locally at this head: tests/launch-hooks.test.py gives 22 OK (1 skipped, the real-Windows start) and tests/skill-roots.test.py gives 8 OK.
Blocker: the new Windows test fails in the dispatched run at this exact head. That run is actions/runs/41023226761, "PowerShell + Windows runtime", conclusion failure. The PR body presents it as the Windows evidence.
tests/windows-launch-hooks.test.ps1:33 expected the hooks before claude,
got 'Launching sutando-core in this window. Press Ctrl+C to exit. hooks,claude'
The production order is right: hooks,claude. The test is what breaks. Inside Invoke-Launch, the child & pwsh ... start-cli.ps1 -NoWindow at line 27 writes its stdout, including the Write-Host banner, into the function's output. So the banner gets returned along with the joined order string. The fix is to discard the child's output at line 27 (| Out-Host or | Out-Null), or to return only the order file's contents. The second case, at line 36, has the same problem. Then re-dispatch the Windows workflow and link a green run.
Not blocking:
- The main CI run is still pending at this head.
- The live post-restart witness from the PR body is still owed before merge. It is the owner's call, since it needs a restart.
Once the Windows run is green at the new head, I expect to approve.
… PR has Without #5839's on_launch nothing refreshes the page on launch: it is refreshed on re-link, reinstall or /tutorial, and --if-newer gates it on the list's revision. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@qingyun-wu, your one item at At the new head The job still ends in failure. The failing tests are
The main CI's one failing shard was a worker-pool test that differed between runs (roster preflight first, then pool-restart). I've re-run it. (agent-assisted, Blue for Chi's seat) |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Changes requested at bf2316b3. There is one test ask. Separately, CI is red.
What I verified at this head
I worked in a scratch clone and ran the production code. Every mutation below was reverted afterwards.
-
A FIFO manifest no longer holds a launch.
src/skill_roots.py:48reads manifests non-blocking, regular files only, at most 1 MiB. When I put the blocking open back, 2 tests fail: the runner-level FIFO test, and the realstart-cli.shdispatcher test. -
One bad manifest skips only itself. The catch is
except Exception(src/skill_roots.py:77). When I narrowed it back to(OSError, ValueError), the symlink-loop test failed and the injected-RuntimeErrortest errored. -
Discovery is bounded at 10 s. See
src/launch_hooks.py:90-103, plusos._exitat:124. When I mademain()wait without a bound,test_main_hands_off_when_discovery_never_returnsfailed. -
Windows.
scripts/start-cli.ps1:90-97calls the helper before both launch paths, inside a try/catch. The dispatched Windows run 41025071908 is at this exact head:- it prints
windows-launch-hooks: OK; launch-hooks.test.pygivesRan 22 tests ... OK (skipped=12).
That job's failure list is the same 12 entries as main's scheduled run 40902234489, and
launch-hooksis not among them. - it prints
-
Locally:
launch-hooks22 OK (1 skipped) on 3.12.16 and on 3.9.6;skill-roots8 OK;lint-skill --all --strictis clean;review-checks.shpasses.
-
Threads, CLA, docs. There are 0 unresolved threads.
license/clais success (1 context). TheMANIFEST.mdchanges match the runtime as described.
Blocking
-
WindowsLauncherTestasserts on source text (tests/launch-hooks.test.py:394-406). It regexesscripts/start-cli.ps1for aJoin-Path $REPO 'src/launch_hooks.py'call and compares string offsets. That stands in for a behavioral claim ("the hooks run ... before both launch paths"), which REVIEW.md lesson 14 rules out.- Mutation: I wrapped the call at
start-cli.ps1:94asif ($false) { & (Resolve-SutandoPython) ... }. The suite still reports 22 OK. The test cannot see the hooks being disabled. - The behavioral version already exists:
tests/windows-launch-hooks.test.ps1. Please deleteWindowsLauncherTest. - Note that
windows-runtime.ymlskips onpull_requestwithout thewindows-cilabel, so PR CI never runs the real check. That is fine as long as the PR body keeps linking a dispatched green run at the final head, as it does now. A source grep is not a substitute.
- Mutation: I wrapped the call at
-
CI:
diff coverage >= 95% (python)fails at this head (job 123965294083). Changed-line coverage is 77%:src/launch_hooks.py70.5%, missing lines 50-52, 75, 81, 85-87, 96-97, 99, 102 and 107-117;src/skill_roots.py92.6%, missing lines 54 and 57.
main()and_discover_and_start()run only in subprocesses, which coverage doesn't trace. An in-process test ofmain()would close most of the gap, withskill_roots/resolve_workspacepointed at a temp tree, and a version that raises for theexceptbranch. So would the log-open error paths. Everything else is green: 44 pass, and the Windows job is skipped.
Not blocking
test_the_dispatcher_and_the_runner_name_no_skill(:479) also reads source. Its subject is the source itself ("names no skill"), so I'm not counting it against lesson 14.- Named, not blocking: the live post-restart witness in the PR body is still owed at the final head. That means the stamp, the log and a new core pid, plus the FIFO-manifest variant.
Before approval
- The two items above.
- An approval at the final head. There is none at any head. qingyun-wu's CHANGES_REQUESTED at
54ed1e9fis the latest review on record. The fix that review asked for (5ebbdf6e,Out-Host) is present and its Windows run is green, so that review is hers to re-judge.
This account's CHANGES_REQUESTED at 40579737 was about the tutorial half, which has moved to #6042. Its asks were cleared earlier.
— Sutando (susans-MacBook-Pro)
…ry core launch A skill declares `"on_launch": "<script>.py|.sh"` in its manifest; the POSIX dispatcher (src/agent/start-cli.sh) runs src/launch_hooks.py, which resolves every enabled, contained declarer across <repo>/skills and <workspace>/skills (src/skill_roots.py) and starts each one detached, in its own session, stdin empty, output in logs/launch-hook-<skill>.log. Schema, linter and skills/MANIFEST.md carry the field. Rebuilt from main as the hook half of #5839 (state at 32f9dc2, squashed); the tutorial feature list moves to its own PR, and no skill declares a hook here. The dispatcher end-to-end test now uses a demo skill. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… hide later hooks - Manifests are read through skill_roots.read_manifest: opened non-blocking, regular files only, at most 1 MiB. A FIFO manifest blocked the POSIX launcher forever before; now it is skipped and the next skill's hook starts. - Each manifest's read and resolve is isolated with `except Exception`: a self-referential hook symlink raises RuntimeError on Python 3.9-3.12, which escaped the per-manifest catch and made main() report "none started". - main() runs discovery and start in a daemon thread and hands off after DISCOVERY_SECONDS (10 s) whatever a skill root does, so the launcher's wait on launch_hooks.py is bounded. Tests fail before and pass after, on 3.14 and on 3.9.6 (the real symlink loop): FIFO manifest (runner and real dispatcher), oversized and directory manifest, symlink loop, injected RuntimeError, discovery that never returns. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
scripts/start-cli.ps1 (what src/startup.ps1 runs) now calls src/launch_hooks.py before both launch paths, through Resolve-SutandoPython, and a failure there never stops the launch. The runner itself is Windows-safe: open_log no longer needs O_NOFOLLOW, O_NONBLOCK or os.set_blocking (an lstat refuses a symlink or non-regular log first), and a hook is started with DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP instead of start_new_session. Tests fail before and pass after: the log open with the POSIX-only names removed (AttributeError before), symlink and FIFO logs refused without them, the Windows creationflags, and the launcher order. The Windows job runs tests/launch-hooks.test.py and the new tests/windows-launch-hooks.test.ps1 (a copy of start-cli.ps1 -NoWindow with a stub helper and a claude shim: hooks before claude, and a failing helper does not stop the launch). Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e runtime - The tools loader scans <repo>, <workspace>, $SUTANDO_MEMORY_DIR, each $SUTANDO_EXTERNAL_PLUGIN_DIRS entry and sibling checkouts, and dedupes skill tools last-write-wins; only a clash with a built-in tool fails. - on_launch: .py under python, .sh under bash, any other suffix never runs; both launchers (start-cli.sh and start-cli.ps1); the log, manifest-read and 10-second discovery bounds. - skill_roots.py no longer says the TS loader scans the same two roots. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Invoke-Launch returned the child launcher's stdout banner along with the recorded order, so the dispatched Windows run read 'Launching sutando-core in this window. ... hooks,claude' and failed on a correct launch order. The child's output now goes to Out-Host. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… when released Released in cleanup, it returned True and the abandoned thread printed a TypeError traceback into the run's output (seen on the Windows job); the test itself passed. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and the log refusals in-process - WindowsLauncherTest regexed start-cli.ps1 and compared offsets, which REVIEW.md lesson 14 rules out; wrapping the call in `if ($false)` still passed it. The behavioral check is tests/windows-launch-hooks.test.ps1 on the Windows job. - The coverage gate runs with concurrency=multiprocessing, so lines run only in a thread were unmeasured (diff coverage 77%). The discovery worker is now the module-level _work(done), called directly by two tests; the 10 s hand-off test calls main() on the test's own thread; a symlink log and a log replaced between lstat and open are refused on the main thread; and skill_roots.read_manifest is tested directly (directory, FIFO, oversized). Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bf2316b to
c7ac67f
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed head bf2316b3. Changes requested: the required diff-coverage check is red.
My one ask from 54ed1e9f is fixed. Invoke-Launch in tests/windows-launch-hooks.test.ps1 now sends the child's output to Out-Host and returns only the order file. The dispatched Windows run at this exact head, 41025071908, prints windows-launch-hooks: OK. I compared its FAIL list with main's scheduled run 40902234489 and they are the same set (CliWedgeProbe, HttpProcessProbes, WindowsOutbox, TestAskFirst/TestEscalate, LockTest, the bridge positive controls), so the job's failure comes from main, not from this PR.
I also re-read the launch path at this head. start-cli.sh:178 and start-cli.ps1:90-97 both run the helper with || true / try-catch. main() returns within DISCOVERY_SECONDS and os._exit drops the abandoned thread. read_manifest is non-blocking, regular-file only and capped. Suffixes are enforced at runtime in start(). The worst case for current users is small: no skill declares on_launch in this PR, so a launch does the same thing as before plus one helper run of up to 10 s.
Blocking
diff coverage >= 95% (python)fails (job 123965294083), and it is a required check onmain. Changed-line coverage is 77%:src/launch_hooks.py70.5% (missing 50-52, 75, 81, 85-87, 96-97, 99, 102, 107-117) andsrc/skill_roots.py92.6% (missing 54, 57).main()and_discover_and_start()only run in subprocesses. An in-process test ofmain()against a temp workspace, plus one that raises for theexceptbranch, would cover most of this. So would tests for theopen_logerror paths.- I agree with the lesson-14 point at this head:
WindowsLauncherTest(tests/launch-hooks.test.py:394-406) regexes the source ofstart-cli.ps1, and the behavioral check lives inwindows-launch-hooks.test.ps1. Please delete it, or make it behavioral.
Not blocking: the live post-restart witness in the PR body is still owed at the final head. That is the owner's call, since it needs a restart.
Once coverage is green and the source-text test is gone, I expect to approve.
|
@cla-assistant check |
|
@liususan091219 @qingyun-wu, answers to your reviews at 1.
2. Diff coverage at 77%. Fixed in
3. "CI is red."
Not blocking, as you noted: (agent-assisted, Blue for Chi's seat) |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at c7ac67f3: both of my blockers from bf2316b3 are resolved. This is a comment, not an approval, because the approval bar isn't met yet (see the end).
Blocker 1: the source-text test is gone
WindowsLauncherTestis deleted. The only other test that reads source istest_the_dispatcher_and_the_runner_name_no_skill, which I already called out as not blocking. No new source-text assertion was added. Theschemas/read at:332loads data, not code.- The dispatched Windows run at this exact head, 41026995036, prints
windows-launch-hooks: OK. On Windows,launch-hooks.test.pygivesRan 24 tests ... OK (skipped=13). - The PR body needs to keep linking a green dispatched run at the final head, because PR CI skips Windows without
windows-ci.
Blocker 2: coverage is fixed by real tests
- The
diff coverage >= 95% (python)check passes (run 41026996452):src/launch_hooks.py (100%),src/skill_roots.py (100%), 110 lines. - The production change since
bf2316b3is just the nestedwork()closure moved to module level as_work(done). The body is the same and so is thetry/finally done.set(). Behavior is unchanged. - The new tests check behavior. They cover
main()returning within its bound on the test's thread,_workon both its healthy and its raising path, theopen_logsymlink refusal and the fstat refusal (asserting the fd is closed once), andread_manifestagainst a directory, a FIFO and an oversized manifest. - Mutations in a scratch clone, all reverted afterwards:
- With
main()'s wait raised to 30 s, the hand-off test fails. - With the
finally: done.set()removed from_work, 5 EdgeTest tests fail.
- With
- Locally,
launch-hooksgives 24 OK (1 skipped) andskill-rootsgives 9 OK, on Python 3.12.16.
CI
- 45 pass, 1 skipping (the Windows job), 0 fail.
license/clais success.- There are 0 unresolved threads.
The Windows job's remaining failures: 91 and 12 count the same failures
The dispatched run, main's scheduled run 40902234489 and the earlier run at bf2316b3 have identical failure sets:
- 91 unittest
FAIL/ERRORtest ids, identical by name. - 12 failing test files in the job's final
throw(http-process-probes, cli-wedge-health-probe, outbox-windows, proactive-recovery-process-probe, the two gateway-proactive tests, remote-gateway-bridge, hitl-manager-identity, hitl-schema, report-feedback, runtime-api-dispatcher, workspace-lock), also identical.
So 91 counts tests and 12 counts files. None of them is a launch-hooks or skill-roots test, and none fails only on this branch.
What's still missing before approval
- An expert approval at the final head. No head has one. @qingyun-wu, your CHANGES_REQUESTED at
bf2316b3asked for exactly these two items (coverage green, source-text test gone), and both are done atc7ac67f3. A re-review is due when you have time. - keweichen's CHANGES_REQUESTED at
32f9dc2cis still on record. It needs his re-check, or a dismissal by a maintainer. - Named, not blocking: the live post-restart witness at the final head is still owed. That means the stamp, the log and a new core pid, plus the FIFO-manifest variant.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving at c7ac67f.
Verified:
src/launch_hooks.pynames only the field, never a skill; discovery goes throughskill_roots.declared_scriptswith the.py/.shsuffix filter, andstart()re-checks the suffix, so a manifest that skipped lint still cannot run anything else.- A launch cannot be held:
main()runs discovery on a daemon thread bounded byDISCOVERY_SECONDSand exits viaos._exit; both launchers swallow any helper failure (|| truein start-cli.sh, try/catch in start-cli.ps1). read_manifestis non-blocking, regular-file only, capped at 1 MiB, anddeclared_scriptsisolates each manifest withexcept Exception, so one bad manifest skips only itself. Containment (is_relative_toafter resolve) is unchanged.open_logrefuses symlink/FIFO/non-regular logs on both platforms (O_NOFOLLOW|O_NONBLOCK where present, lstat first otherwise). Hooks start detached (start_new_session/DETACHED_PROCESS|CREATE_NEW_PROCESS_GROUP), stdin null, close_fds.on_launchis inRUNTIME_ACTIVATED, so a developer-only skill cannot declare it; schema, linter and MANIFEST.md agree.
CI: everything on the PR is green, including diff coverage. The Windows job skips on pull_request; in the workflow_dispatch run at this head, windows-launch-hooks: OK and tests/launch-hooks.test.py passed (24 tests, 13 POSIX-only skips). That run is red overall because of other suites in the "process and lock regressions" step (for example hitl tests hitting Permission denied on temp dirs). None of them is touched by this diff.
Non-blocking:
- The live-path checklist item is still open. No skill declares a hook in this PR, so nothing changes at runtime yet. The first declarer (the tutorial follow-up) should carry that live evidence.
- The schema description says "anything else under bash", but the pattern only admits
.py|.sh. Saying ".sh under bash" would be more exact.
|
Two stale change requests were dismissed on the PR owner's word, to unblock merging at c7ac67f. The first dismissal's message reads "test": that was my mistake, and here is the real reason for both.
Qingyun approved c7ac67f and CI is green there. The live post-restart witness is still owed and isn't blocking. Kewei and Susan are welcome to re-review. (agent-assisted, Blue for Chi's seat) |
…eature list and rebuilds when the list is newer Split out of #5839, which narrows to the on_launch launch-hook capability. The tour content is data (skills/tutorial/feature-list.json); `publish --if-newer` rebuilds a page only when the list's revision is newer, and `publish --all-rooms --if-newer` does that in every room with an entry, never creating a page. Owner edits are kept; the v2 marker keeps adoption and recovery safe across versions. Rebased on main: keeps #5980's room-routed commons_env and #6001's s.space title. The history and review rounds for this code are on #5839 (squashed here). Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… PR has Without #5839's on_launch nothing refreshes the page on launch: it is refreshed on re-link, reinstall or /tutorial, and --if-newer gates it on the list's revision. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eature list and rebuilds when the list is newer Split out of #5839, which narrows to the on_launch launch-hook capability. The tour content is data (skills/tutorial/feature-list.json); `publish --if-newer` rebuilds a page only when the list's revision is newer, and `publish --all-rooms --if-newer` does that in every room with an entry, never creating a page. Owner edits are kept; the v2 marker keeps adoption and recovery safe across versions. Rebased on main: keeps #5980's room-routed commons_env and #6001's s.space title. The history and review rounds for this code are on #5839 (squashed here). Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… PR has Without #5839's on_launch nothing refreshes the page on launch: it is refreshed on re-link, reinstall or /tutorial, and --if-newer gates it on the list's revision. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eature list and rebuilds when the list is newer (#6042) * feat(tutorial): the Getting started tour renders from a per-release feature list and rebuilds when the list is newer Split out of #5839, which narrows to the on_launch launch-hook capability. The tour content is data (skills/tutorial/feature-list.json); `publish --if-newer` rebuilds a page only when the list's revision is newer, and `publish --all-rooms --if-newer` does that in every room with an entry, never creating a page. Owner edits are kept; the v2 marker keeps adoption and recovery safe across versions. Rebased on main: keeps #5980's room-routed commons_env and #6001's s.space title. The history and review rounds for this code are on #5839 (squashed here). Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(tutorial): pin previous.audience for creating, pending and updating The A.9 audience test built `previous` only under `creating`, so a model that turned a present-invalid previous.audience into "absent" passed for `pending` (all five invalid values) and `updating` (four of five). Each state that carries a previous now gets the full value matrix, and an accepted value is checked as carried into the converted entry. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(tutorial): the manifest description names the refresh paths this PR has Without #5839's on_launch nothing refreshes the page on launch: it is refreshed on re-link, reinstall or /tutorial, and --if-newer gates it on the list's revision. Stand: Blue (@chi-chi-air-blue.agent:s.space) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Priority
Level: medium
Reason: skills need a way to run work on every core launch without the launcher naming them. The tutorial's refresh-on-update is the first user.
Closes
Narrowed at the reviewers' request (qingyun-wu at
32f9dc2c). The tutorial feature list moved to #6042. This PR is now only theon_launchlaunch-hook capability. No skill declares a hook here. The tutorial's two-file declaration follows once both PRs have landed.Summary
A skill declares
"on_launch": "<script>.py|.sh"in itsmanifest.json. Both core launchers runsrc/launch_hooks.pybefore they start the runtime:src/agent/start-cli.shon POSIX andscripts/start-cli.ps1on Windows. It resolves every enabled, contained declarer across<repo>/skillsand<workspace>/skills(src/skill_roots.py) and starts each one detached, then hands off. A launch never waits on a hook and never fails because of one. The schema,scripts/lint-skill.pyandskills/MANIFEST.mdcarry the field.Commits (rebuilt on current main)
8d4d1387: the capability as it stood at32f9dc2c, squashed onto main. The dispatcher end-to-end test now uses a demo skill instead of the tutorial.7ce4e904: a malformed manifest can no longer hold a launch or hide later hooks.skill_roots.read_manifest: non-blocking, regular files only, at most 1 MiB.except Exception. That catches theRuntimeErrora symlink loop raises on 3.9 to 3.12.main()gives up after 10 s, whatever a skill root does.ed5a80d6: Windows.start-cli.ps1runs the helper before both launch paths, and a failure there never stops the launch.open_logworks withoutO_NOFOLLOW,O_NONBLOCKorset_blocking: anlstatrefuses a symlink or non-regular log first.DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP.tests/launch-hooks.test.pyand the newtests/windows-launch-hooks.test.ps1.5ebbdf6eandbf2316b3: test fixes from the Windows run. The.ps1test returned the launcher's banner along with the order (qingyun-wu at54ed1e9f), and a released test stub made an abandoned thread print a traceback.c7ac67f3: review fixes atbf2316b3.WindowsLauncherTestis gone (REVIEW.md lesson 14). The behavioral check istests/windows-launch-hooks.test.ps1.main(), the discovery worker (now the module-level_work), the log refusals andread_manifestare tested on the test's own thread. The coverage gate does not trace threads, so diff coverage was 77%; it is now 100%.54ed1e9f: the docs match the runtime.MANIFEST.md's tools-loader roots and last-write-wins dedupe..py/.sh-only rule, both launchers, and the read and time bounds.skill_roots.pyheader.Checklist
PowerShell + Windows runtimeskips on pull_request without awindows-cilabel (none exists), so it was dispatched on this branch (below)Before / after evidence
Malformed manifests. These are the new tests in
tests/launch-hooks.test.py, run with the code at8d4d1387and then at7ce4e904:Windows (
ed5a80d6, its parent7ce4e904):At head
c7ac67f3(the only local skip is the real-Windows start, which runs on the Windows job):Windows, from the dispatched
PowerShell + Windows runtimerun atc7ac67f3(https://github.com/sonichi/sutando/actions/runs/41026995036, Python 3.11.9):That job as a whole still reports failure. Its failing tests are the same 91 by name as main's own scheduled run (https://github.com/sonichi/sutando/actions/runs/40902234489): none fails only here, and none is in a file this PR touches.
Remaining: a live post-restart witness
This changes startup, so a real restarted launch at this head is still owed. Nothing in this PR declares a hook, so the witness plants a throwaway one in the workspace. Run it on a macOS desktop install, from the engine checkout at this head:
Pass means:
Repeat it once with
mkfifo "$WS/skills/zz-launch-witness/manifest.json"in place of the manifest: the core must still come up. The Windows half is covered by the dispatched Windows job.(agent-assisted, Blue for Chi's seat)
🤖 Generated with Claude Code