Skip to content

Give the manager a Settings shortcut, and take the second out of it - #5

Closed
CaffreySun wants to merge 92 commits into
dncore:mainfrom
CaffreySun:pr/settings-shortcut
Closed

CaffreySun wants to merge 92 commits into
dncore:mainfrom
CaffreySun:pr/settings-shortcut

Conversation

@CaffreySun

Copy link
Copy Markdown
Contributor

What this does

The app menu gains Settings… on ⌘,, which puts the manager on its Settings
section — opening it if it is closed, re-fronting it if it is not.

It is in the main menu and only there. The status menu deliberately carries no
key equivalents (makeMenu): an accessory app with a nonactivating pet panel is
never the active one, so a shortcut there would never fire while advertising
itself. The main menu is displayed exactly when the app is frontmost, which
for this app means exactly when the manager window is open — so the case ⌘, is
for is "already in the manager, wanting Settings". A global hotkey would reach
the pet on the desktop, and would take ⌘, away from every other app on the
machine; not doing that.

The second it used to take

A user reported that pressing ⌘, took about a second before Settings appeared.
Layered timing found three synchronous main-thread costs stacked in the press:

cost measured
openManager() → refreshAll() → refreshAgents() spawns one --version process per registered agent 445–705ms
MainWindowView.onAppear called refreshAll() again, so opening paid for agent detection twice same cost, twice
show() rebuilt the window's whole view tree for a window that was already on screen 200–357ms

None of it is needed to switch a section, and because it all runs before the
window can repaint, the section appeared only after it finished.

Now show() rebuilds a fresh tree only for a window that is not on screen —
the case the rebuild exists for, since a closed window stops receiving SwiftUI
updates; the view no longer refreshes on appearing, so one open costs one
refresh, in the code that opens the window; and openSettings() re-fronts an
open manager rather than re-opening it.

Measured: 0–2ms for the press, against 652ms for the same press before.
Opening from closed still pays what it always did (agent detection 0.5–0.6s
plus the window's first build 0.3s; 2.0s on the coldest run in the process).

Looks unrelated, but is this PR's business

Four things a reviewer will want an explanation for:

  • show() now rebuilds conditionally at all. This affects every path that
    opens the manager, not just ⌘,: closing and reopening still gets a fresh tree,
    re-fronting an open one does not.
  • .onAppear { model.refreshAll() } is gone from MainWindowView. One
    refresh per open, in the code that opens the window, instead of two. That
    halves the cost of the "Open Pet Manager…" menu item as well.
  • A diagnostic run counts as open. --selftest and friends never put the
    manager on screen (presents = !HeadlessMode.isActive), which makes "on
    screen" permanently false there; the guard therefore reads a window that
    exists in HeadlessMode as open. There is no screen to keep alive, and
    reading it as closed would rebuild the tree between the two presses the
    shortcut's own check makes — measuring a rebuild no user can cause.
  • The self-test finds its window by identity, not by title. A closed manager
    window stays in NSApp.windows (isReleasedWhenClosed = false) and a
    diagnostic run puts none of them on screen, so "first window with this title"
    can be the previous check's — the sidebar check opens one of its own.

Self-test coverage

checkSettingsShortcut drives the real main menu with a real key event —
NSApplication.sendEvent first, NSMenu.performKeyEquivalent(with:) only if
that delivers nothing, with the route and the action's duration printed rather
than assumed — and asserts that the item is offered on ⌘ alone, that a closed
manager opens on Settings, that an open one moves the same window there
without rebuilding its view tree (contentViewController identity) and
without holding the main thread for more than 250ms, and that a focused
Settings field does not eat a menu shortcut (proved with the window's other
shortcut, ⌃⌘S, since the sidebar is observable where the section is not).

Four negative controls, each run and reverted: pointing the item at
openManager fails two assertions; deleting the item reports that no ⌘, item
exists; putting the rebuild back reports a 357ms rebuild; making the layout
ignore row.alignment turns the focus assertion's scaffold into a failure.

What this does not do

  • No global hotkey (see above).
  • Agent detection on the manager's open path is still synchronous; moving it off
    the main thread is its own change.
  • ⌘, with the manager minimized is unverified — this machine refuses to
    minimize a window from a diagnostic run (miniaturize(nil) leaves
    isMiniaturized false), so the self-test prints – not checked rather than
    guessing, and Apple's own documentation does not say whether
    makeKeyAndOrderFront deminiaturizes.

Verified

  • swift build --build-tests, swift test --parallel: 579 tests in 88
    suites
    .
  • The rest of the CI gate set run locally: the shim contract, the bundle and
    plutil -lint, --diagnose/--status/--help exiting 0, and the leak scan
    (no personal paths, no credentials).
  • --selftest passes on the assembled bundle and on the copy installed into
    /Applications; the timings above come from its own printed numbers.

Docs: ARCHITECTURE.md §6.5e — the decision, the timing, the assertions and
their counter-evidence, and the diagnostic-run nuance — plus both READMEs'
status tables.

One note on scope: a pre-existing defect in the old alignment self-check (it
built its "before" case from the user's own alignment, so a user who picked
right made it compare right with right and fail) surfaced while reviewing
this work. It is already fixed on main by eb8f003/a752667, which rewrote
that check to state both alignments explicitly — so nothing for it is carried
here.

dncore and others added 30 commits September 12, 2026 09:58
Reviews Agent_Pet_Runtime_v0.1.md against the actual integration
surfaces of the four target agents and the real pet packages on disk.

Key corrections:
- The event bridge is not a server agents connect to; three of four
  agents deliver events by spawning a process. Adds the missing shim.
- Hooks sit on the agent's critical path; adds the latency and
  never-block contract the spec omitted.
- Introduces the AgentState -> atlas row mapping the spec lacked, and
  drops the redundant PetState layer.
- V2 atlases (1536x2288) now degrade gracefully instead of being
  rejected, since such pets are already installed locally.
- Gives the Activity Engine the concrete timings it only named.

Implements PetManifest, CompatibilityProfile, AnimationTrack and
ActivityEngine with 81 tests, fixtures copied verbatim from
~/.codex/pets/.

Co-Authored-By: Claude Code <[email protected]>
Implements the validation stage the spec named but never defined.

- AtlasValidator enforces the rule the spec omitted: cells past a row's
  frame count must be fully transparent, else leftovers bleed into
  shorter rows during playback.
- AtlasBitmap abstracts pixel access so the rules are testable against
  synthetic bitmaps, with no image files or 11MB allocations.
- PayloadValidator uses an allowlist, lenient enough for the .txt,
  .DS_Store and run/ directories real packages contain.
- PathSafetyValidator normalises lexically before deciding, so
  ./images/../sheet.webp is accepted while genuine escapes are not.

122 tests.

Co-Authored-By: Claude Code <[email protected]>
Loads and validates real pet packages end to end.

Atlas residue severity is calibrated against the real corpus rather
than copied from the authoring toolchain. OpenAI's validate_atlas.py
fails a build on any stray pixel in a surplus cell, but that tool gates
authoring while this gates installation, and the two questions differ:
the renderer only samples columns 0..<frameCount, so surplus pixels are
unreachable and cannot bleed into playback. Residue is therefore a
warning, because refusing to install a working pet is the worse error.

That call is backed by measurement, not assumption. Three installed
pets are clean; pet-ben-hill carries 13560 stray pixels in idle's
seventh column and still installs. Asserting the stricter rule would
have rejected a pet the user already runs.

143 tests, including integration tests against every package in
~/.codex/pets that no-op when the directory is absent.

Co-Authored-By: Claude Code <[email protected]>
An NSPanel-backed desktop pet that loads real Codex packages, slices
their atlases, and animates from the activity engine. Draggable, with
position persistence that refuses to restore onto a detached display.

The frame on screen is a pure function of what the engine focuses on,
so the animation preview and a live agent reaction cannot diverge --
there is no separate demo path to fall out of sync.

Adds --diagnose and --selftest because screenshotting is unavailable
here: without Screen Recording permission, screencapture returns only
wallpaper, so nothing visual can be verified that way. The self-test
draws each state through the live view and counts non-transparent
pixels in the backing store instead.

The self-test immediately caught a real bug: a stale waving gesture
from `completed` bled into every subsequent state, so failed/paused/
unknown all rendered the same celebration. It also caught an error in
its own first assertion, which flagged waitingInput and waitingApproval
for rendering identically when sharing a track is exactly what they
should do.

Co-Authored-By: Claude Code <[email protected]>
Implements the piece the original spec left undefined and the one whose
design was most wrong: the bridge is not a server agents connect to, it
is a shim they execute plus a socket it writes to.

Shim constraints, in priority order: always exit 0, never block, never
write to stdout. Claude Code reads a non-zero hook exit as meaningful
and will alter agent behaviour, so a crashed pet must be invisible
rather than an error. Verified end to end: a payload sent by the real
binary arrives as a typed event driving the activity engine, and the
shim exits 0 even with no runtime listening.

Latency is the binding constraint, so it was measured before the design
was fixed rather than after. Swift with Foundation starts in 3.72ms
against C's 2.23ms; the 1.2ms buys a JSON envelope that can be debugged
with nc -U. Against a live runtime: P50 4.33ms, P99 6.59ms, max 6.90ms
over 500 samples, inside the 5ms/25ms budget with nothing above 8ms.

Normalization is a table, not code, so adding an agent does not mean
touching the runtime core. Session identity falls back to the shim's
parent pid -- the agent -- which keeps concurrent sessions of one agent
apart where a constant would merge them.

Two bugs the tests caught: hand-built JSON broke on any path with an
apostrophe, because shell quoting emits escapes that are invalid in
JSON; and the socket path guard, written for sun_path's 104-byte limit,
immediately fired on the test's own temp paths.

199 tests.

Co-Authored-By: Claude Code <[email protected]>
The three pieces that were missing, each built around the question of
what could go wrong rather than what should happen.

Config transaction: snapshot, back up, edit in memory, re-check, write
atomically, verify, restore on any failure. It exists because
~/.claude/settings.json is written by Claude Code too, and a malformed
version of it breaks the user's agent. Idempotency is judged on parsed
meaning rather than bytes, since serialising reformats the file and a
byte comparison would report a change on every run.

Agent integrations: hook installation is record-driven, so removal
matches the command actually written rather than the current shim path.
A runtime that has since moved can still clean up after itself. A moved
runtime replaces its stale hook instead of leaving two.

Pet store: install copies into staging and validates before pets/ is
touched at all, so a rejected package leaves no trace — rollback with
nothing to roll back. managedByRuntime is the single field deciding
whether uninstall may delete, which is what makes "never delete the
user's package" implementable rather than aspirational.

Two bugs found by the tests. Uninstall left `"PreToolUse": []` where the
key never existed, so a round trip did not restore the file's meaning.
And PetPackageLoader reported a manifest that was present but malformed
as "manifest not found", sending the user to look for a file sitting
right there — now a distinct error.

Of four agents, only Claude Code is configurable. Grok's hooks are TOML
and Codex's notify is an argv array, neither of which the JSON
transaction can edit safely; they are detected but report no configure
capability rather than being half-supported.

263 tests.

Co-Authored-By: Claude Code <[email protected]>
The manager UI: import, preview, upgrade and uninstall pets; detect and
configure agents; watch live sessions.

Pet previews drive AnimationResolver, the same resolver the desktop pet
uses, so the manager cannot drift from what the user actually sees.
Atlases are decoded once per pet and cached, because re-decoding an
11 MB sheet on every redraw would make the window unusable.

Agent cards recompute health from reality on every refresh and store
nothing derived. A cached "connected" flag would be wrong the moment the
agent quit. Confidence is surfaced in the activity list, since a
process-observed state is a guess and a hook-reported one is not.

An agent with no configurator says so instead of offering a button that
would fail. Grok's hooks are TOML and Codex's notify is an argv array;
neither is something the JSON transaction can edit safely, so they are
detected and reported as not configurable.

Two bugs the tests caught. IntegrationStore encoded dates as ISO-8601
but decoded with a plain decoder, so records written successfully read
back as empty and uninstall silently found nothing to remove — encoder
and decoder now come from one place. And the first sensitivity test
searched for the substring "prompt", which matches the legitimate event
name UserPromptSubmit; it now asserts a closed schema instead.

269 tests.

Co-Authored-By: Claude Code <[email protected]>
Fills the last acceptance criterion and makes everything the GUI does
available from a terminal.

Launch at Login uses SMAppService, which identifies the app by its
bundle. Run from `swift run` there is no bundle, so it reports
"not available" rather than failing silently — a toggle that does
nothing is worse than one that explains itself.

The diagnostics bundle records only versions, integration status, and
state transitions. It cannot leak a prompt or a credential, because the
runtime never reads either in the first place; the test asserts a closed
schema rather than searching for substrings, which would have flagged
the legitimate event name UserPromptSubmit.

AppConfig treats a file from a future schema as absent rather than
partially applying it: half-understanding a newer config is worse than
starting from defaults. autoConfigureNewAgents stays off by default —
software that edits agent configuration on first sight is software the
user cannot trust.

New headless flags: --status, --configure, --unconfigure.

283 tests.

Co-Authored-By: Claude Code <[email protected]>
The pet rendered and animated perfectly but could not be moved. Two
separate causes, and the first alone was fatal.

NSView.acceptsFirstMouse(for:) defaults to false. AppKit's default
delivers the first click in an inactive window to the window for
activation and not to the view — correct for most windows, wrong for a
desktop pet, which the user clicks precisely because they are working in
another app. An accessory app is essentially never active, so every
click was spent activating and none reached the view.

The drag arithmetic was also wrong. It differenced
NSEvent.locationInWindow, but the window's coordinate system moves with
the window, so each event was measured against an origin the previous
event had already changed and the pet would have accelerated away from
the cursor. Now screen-space, and extracted to WindowDrag where it is
pure and tested.

Adds clamping so a pet cannot be dragged fully off-screen — it has no
dock icon and no window list entry, so there would be no way to get it
back — and the same check now gates position restore.

The self-test gained a drag-readiness section, because this failure is
silent: the pet looks correct whether or not it can be moved.

295 tests.

Co-Authored-By: Claude Code <[email protected]>
Replaces several guesses with the real contract, found on disk at
/Applications/ChatGPT.app/.../hatch-pet/references/.

Two corrections that invalidate earlier work. Durations are per frame in
milliseconds and are not uniform -- idle runs 280,110,110,140,140,320,
holding its ends two to three times as long as its middle. That is a
breath; a single fps cannot express it, and AnimationTrack no longer has
one. And V2's rows 9 and 10 are not reserved capacity but sixteen
clockwise gaze poses at 22.5 degrees, 000 being straight up, with
forward as a deadzone that falls back to idle.

That second one answers a question the spec review had marked
unverified. Confirmed against real art: exporting all sixteen poses from
the installed V2 pet shows a continuous clockwise turn from straight up
through right, down, and back.

Playback is now layered: dragging, then a playing gesture, then the
agent's state, then gaze, then idle. Dragging takes locomotion rows so
the pet walks while carried, and reverses when the drag reverses.
Vertical drags keep their direction, since the atlas has no row for
walking straight up and flipping on cursor wobble looks like a fault.

The mapping the contract does not state is now documented per row with
its reasoning. completed settles on `jumping` -- anticipation, lift,
peak, descent, settle -- and waving becomes a greeting for when the pet
is clicked, since a greeting is a reaction to the user rather than to
the agent.

Two bugs the new tests caught: only `failed` fell through after its
one-shot, so a finished celebration froze on its last frame forever; and
returning the idle track at the state layer ended the decision before
gaze ran, so the pet would never look anywhere.

321 tests.

Co-Authored-By: Claude Code <[email protected]>
Reported as "how do I open the pet manager?", which is the finding:
the only route was a menu bar icon, and nothing on screen pointed to
it. The pet is the thing the user is looking at, so it is where they
reach.

Right-clicking the pet now raises the same menu as the status item, and
double-clicking opens the manager outright. The menu is built by one
function so the two routes cannot drift.

Also removes the shortcut from "Open Pet Manager". It was bound to Cmd-O
and could never have fired: this is an accessory app, so it is never the
active application, and menu key equivalents only dispatch to the active
one. Offering a shortcut that does nothing is worse than offering none.

Co-Authored-By: Claude Code <[email protected]>
Reported as "several Claudes are running and it shows waiting for input,
I don't know whether that's right". It was not right, and the cause was
a mapping built on guesswork.

Ground truth came from three places: the complete hook event list
extracted from the installed claude binary (2.1.268 dispatches fifteen,
I was hooking seven), real payloads captured with --log-events using my
own session as the subject, and Otty.app's shipped implementation for a
cross-check.

Notification is a grab bag distinguished only by notification_type,
whose values are permission_prompt, idle_prompt, auth_success and
elicitation_dialog. Mapping all of it to waitingInput -- a state that
never expires -- is why the pet got stuck asking for attention as soon
as more than one session was open. Only permission_prompt now maps, and
through PermissionRequest, which is the one true "blocked on you"
signal. The rest change nothing.

Stop fires at the end of every turn, not at the end of a session, so it
now settles to idle after the celebration instead of sitting at
completed forever. TaskCompleted is the event that means a task
finished. StopFailure distinguishes a failed turn from a successful one.
And a Stop with a still-running background subagent stays working rather
than announcing completion mid-job.

Also fixed: Grok Build scans and trusts ~/.claude/settings.json, so the
Claude hooks installed there run on Grok events too. The shim now reads
GROK_HOOK_NAME and re-labels those, instead of reporting every Grok
session as Claude Code.

Reconfiguring installed 14 hooks. The three other tools already using
those hooks -- a Yeelight bridge, iTerm2 status, and Otty -- are intact.

336 tests, including a scenario suite replaying the reported situation.

Co-Authored-By: Claude Code <[email protected]>
A security review flagged the capture file as world-readable. The mode
was wrong, but the content was the real problem: --log-events wrote
whole hook payloads verbatim, including tool_input and tool_response --
the commands an agent ran, their output, and file contents. This
project's own review document says prompts, model output, and source
code are never recorded, and the feature I added did exactly that.

Fixed by dropping content rather than just tightening permissions. A
capture record now keeps only the fields diagnosis needs: which session,
which event, which tool by name, the notification type, and the shape of
any background tasks. Unknown keys are dropped rather than copied, so a
field a future agent build adds cannot leak into a log by default.

Verified against a real payload: the command text is absent, and
tool_input appears only as a name in _omitted_keys with no value beside
it. Field names are kept because a reader needs to tell a removed field
from an absent one, and a name is schema rather than content.

Two things the log does still contain, both deliberate and both needed
to correlate sessions: session_id, an opaque UUID, and cwd, the project
directory. It is written 0600, and enabling it now prints what it does
and does not record.

Key names are not content, so the redaction test asserts on values -- an
earlier version of it forbade the names too, which would have made the
omission list impossible.

347 tests.

Co-Authored-By: Claude Code <[email protected]>
Sanitisation first, because publishing is the one step that cannot be
quietly undone.

The test fixtures were copies of real packages, including one built from
a named individual's likeness. All four are now synthetic, each still
reproducing a shape that actually occurs: a minimal manifest, one with
third-party ecosystem fields, a V2 package whose id differs from its
folder, and a non-ASCII display name. The docs are genericised the same
way.

Adds the packaging and release machinery: an app bundle builder, an
icon generated to Apple's macOS grid (824pt squircle centred in 1024,
corner radius 185.4) and verified at Dock sizes down to 32px, MIT
licence, a .gitignore, and three workflows.

CI gates on more than tests. The shim is checked to exit 0 both with
and without a runtime listening, since that is its most important
property and the easiest to regress. A second job fails the build if a
real home directory or a credential-shaped string reaches a tracked
file, which is a rule best enforced by machine rather than by care.

The release workflow builds the bundle, packages it with ditto so the
bundle's symlinks survive, and publishes. The Homebrew job rewrites the
tap formula with the new version and checksum, and skips with a notice
rather than failing the release when the tap token is absent.

Co-Authored-By: Claude Code <[email protected]>
The workflow scans *.yml, which includes itself, and it contains the
literal github_pat_ it searches for. Every build would have failed on
the linter rather than on anything real.

Excludes the workflow directory and rebuilds the check around a helper
so a match is reported rather than merely gating the exit code. Verified
with a negative control: planting /Users/realperson in a source file
does make it fail.

Co-Authored-By: Claude Code <[email protected]>
dncore/homebrew-tap holds formulae at the repository root alongside
wgtun.rb, not in a Formula/ subdirectory. Checked the existing tap
rather than assuming the layout.

Co-Authored-By: Claude Code <[email protected]>
CI failed on the first push: macos-15 runners ship Swift 6.1 and the
package declared 6.2. Nothing here actually needs 6.2 — it builds and all
347 tests pass under 6.1, so the declaration was aspirational rather than
required.

Left a note at the declaration explaining the floor, since raising it
again would silently break the default runner image.

Co-Authored-By: Claude Code <[email protected]>
The first green CI run was cancelled at twenty minutes, and the steps
show why: build, test, shim check and bundle check all passed, then
Smoke-test the app was cancelled. It had hung.

With no pet installed -- as on a runner, or over ssh -- startup called
NSAlert.runModal to explain the absence, and nobody was there to dismiss
it. A diagnostic that needs a human is not a diagnostic.

Modal alerts are now suppressed when any non-interactive flag is
present, and the missing-pet case reports to stderr instead. The
self-test reports SKIPPED rather than failing when there is nothing to
render, since that is an environment fact and not a defect.

--help turned out to be the same bug in a different coat: unrecognised
flags fell through to the GUI, so --help launched a pet and never
returned. It and --version are now real flags.

CI gained a guard for exactly this: every diagnostic flag is run with a
two-minute timeout, because these are what bug reports and CI use and
any of them hanging is a defect.

Co-Authored-By: Claude Code <[email protected]>
CI caught this: --diagnose exited 1 on the runner. It has no display, so
the command reported "no displays are available" and treated that as a
failure.

But reporting a missing display, or a missing pet, is the diagnosis. A
non-zero exit says the command did not work when in fact it did exactly
its job — and it fails a smoke test on any bare machine while telling
you nothing about whether the build is sound.

Reserved non-zero for genuinely not being able to diagnose at all.

Co-Authored-By: Claude Code <[email protected]>
A .app is what Homebrew calls a cask; a formula is for something that
builds from source into a prefix. Installing a GUI bundle as a formula
puts it somewhere the Finder never looks. Modelled this on
dncore/axon-llm-dispenser, which has been doing it for a while.

Two things that cask does and mine did not, both of which are bugs a
user would hit:

preflight_steps quits the running app before the bundle is replaced.
Without it the old binary keeps running from memory while holding the
bridge socket, so hook events keep going to a process whose files are
gone. postflight_steps strips the quarantine attribute, without which
Gatekeeper reports an ad-hoc signed download as damaged.

The cask now lives here as a template and is pushed to a dedicated tap
by the release workflow, rather than being assembled inline in YAML
where nobody would notice it changing. Generated with Homebrew's own
autocorrect rather than by hand: stanza order put caveats last, and
depends_on :macos was required.

And the caveats say something specific to this app: uninstalling does
not remove the hooks it wrote into your agents' config files, so those
have to come out first or they keep invoking a shim that is gone.

Co-Authored-By: Claude Code <[email protected]>
Personal Claude Code context for this repo lives in CLAUDE.local.md, which
Claude Code loads alongside CLAUDE.md at the start of every session in this
directory. It stays local on purpose: the repository is public and the file
carries local working notes rather than anything a contributor needs.

Co-Authored-By: Claude Code <[email protected]>
`AppConfig.pet.defaultPetID` has existed since settings landed, documented
as "the pet shown on the desktop", and nothing ever wrote it or read it:
launch always took `library.first`, and picking a pet from the menu bar
or the manager changed only what was on screen. Quit the app and the
choice was gone, which is what a user reported after installing the
release from the cask.

Both places a pet can be chosen now record the id through `AgentPetModel`,
so there is one writer and the file cannot be half-updated. The menu bar
passes an id rather than an `InstalledPet` because it lists pets found in
`~/.codex/pets` that were never imported into the runtime.

At launch the saved id is resolved against the library as it exists now.
A pet that was uninstalled, or whose folder was renamed, simply does not
match and the first pet is used, rather than starting with nothing. The
stale id is deliberately left on disk: reinstalling the pet restores the
choice. `--pet` remains a per-run override and is not written back, so a
diagnostic run cannot overwrite what the user picked.

Two smaller things in the same path: the manager now marks the restored
pet as the one on the desktop, which it could not do before because the
pet was chosen before the model existed, and `--verbose` prints which
pet was selected, which is what made this verifiable from a terminal.

Co-Authored-By: Claude Code <[email protected]>
The runtime used to copy pets into its own store under Application
Support, with provenance metadata, a staging install transaction, and
Import / Upgrade / Uninstall in the manager. That made two lists of what
is installed, and the two drift: a pet installed with Codex's own
tooling only appeared here after an Import, and the imported copy went
stale the moment the Codex-side one changed.

Pets now come from one place, the directory Codex itself reads:
$CODEX_HOME/pets, or ~/.codex/pets when CODEX_HOME is unset. CODEX_HOME
replaces the default rather than adding to it, matching the codex-pets
CLI and the hatch-pet skill; searching both would list a pet the
terminal Codex cannot actually load. The directory is read-only — the
runtime never writes to it.

Adding, updating, and removing belong to Codex's own tooling.
`npx codex-pets add <pet-id>` installs and, run again, updates, because
the CLI just overwrites pet.json and spritesheet.webp; removing a pet is
deleting its folder, since nothing in that ecosystem ships a remove
command and inventing one here would be a second, quieter semantics.
The Pets view lists the directory, previews each pet, and puts the
chosen one on the desktop; its detail pane shows those exact commands
instead of buttons that would reimplement them.

Deleted with the store: PetStore, PetInstallMetadata, the four manager
actions, and their twenty tests — about 1100 lines. Loading and
validation are untouched, so a package Codex would refuse is still
skipped rather than half-rendered.

Also fixed while in there: the Pets view's compatibility-warning row now
comes from the full load rather than the discovery listing, which never
carries warnings; it had been showing "None" for a package that does
deviate from the contract.

Breaking for anyone who imported pets into the old store: those copies
are no longer read. Reinstall with `npx codex-pets add`, or drop the
package folder into ~/.codex/pets.

Co-Authored-By: Claude Code <[email protected]>
Three things, all of them found by comparing against Codex's own sources
rather than guessing at strings in the binary.

**The message element.** Codex's ambient pet carries a notification —
Running / Needs input / Ready / Blocked, a fallback second line, and a
lifetime per kind. Its terminal PR (#21206) says outright that the text
overlay is disabled, so the model is authoritative and the drawing is
ours: a bubble above the sprite, with the window growing upward and the
origin left alone, which is the desktop equivalent of Codex reserving
terminal rows above the pet. Labels, bodies, lifetimes and the
one-line-vs-two-line rule are Codex's constants, not invented here.

One body is deliberately missing. Codex fills exactly one — the
assistant's message preview on `review` — from a model stream this
runtime is not: the shim forwards event metadata and never model output.
The second line carries what the events do have (which tool is waiting,
what failed, both of which Codex's own desktop notifications show too).
It never carries the user's prompt, which is what the running event's
summary actually holds.

**The avatars directory.** `avatar.json` is not an alternative manifest
name; it is a second directory Codex still reads,
`$CODEX_HOME/avatars/<folder>/`. Both are scanned, pets/ last so a
folder present in both resolves to the newer package, and the manifest
falls back the way Codex's loader does: id to the folder name,
displayName to the id, spritesheetPath to the conventional file. Loading
by path now accepts either manifest name, as Codex's does.

**The self-test's identical-track check was wrong.** It compared a
single frame at t=0 by opaque-pixel count. clippit opens every one of its
rows with the same neutral pose — measured, not assumed — so five
distinct animations were reported as duplicates. Renders are now
compared by content hash at two points per animation, which passes
clippit and is strictly stricter elsewhere.

Verified against the real thing: a PermissionRequest through the actual
shim logs `Needs input — Bash`, Stop logs `Ready`, and the message clears
when the session settles. Self-test passes on a V1 pet, a V2 pet with
gaze, and clippit.

Co-Authored-By: Claude Code <[email protected]>
Three things, each settled against Codex's own code rather than against
what the published contract implies it should do.

**Playback.** The app's pet implementation lives in the desktop app's
asar, and it settles two questions the TUI alone could only suggest.
Idle is authored as 280/110/110/140/140/320ms but *played* six times
slower (`ger = her.map(frameDurationMs * 6)`, and `uer('idle')` returns
`ger`); the TUI's `idle_animation` carries the same six-times numbers.
And every non-idle state plays its row three times and then hands over
to the idle row, which is where the loop restarts — our pet repeated the
working row for as long as the agent worked, which is six times too fast
and never calms down. Both are ported: idle moves to the played timings,
the resolver settles after `repeats` passes, and `animationPlaysOnce`
and `settledState` disappear because settling is the whole mechanism.

Two deliberate deviations, both documented: locomotion still loops for
as long as a drag lasts (Codex's drag is a different machine, and a pet
that settled mid-drag would look broken), and waving/jumping remain the
click-gesture layer rather than three-pass states.

`completed` now plays row 8 `review`, which is the row Codex plays when
a turn completes — its Review notification is this exact state. Reduced
motion collapses every layer to its first frame, and the
`respectsReduceMotion` setting, until now a switch nothing read, is what
decides whether that applies.

**The assistant's preview.** Codex fills one notification body from the
assistant's message; Claude Code hands hooks `last_assistant_message` on
`Stop` precisely so nothing has to read a transcript, so the body is now
filled the same way Codex fills it: whitespace collapsed and cut to two
hundred characters, in memory only. It is deliberately absent from the
event-capture allowlist, and a test asserts both that it stays off the
list and that a payload carrying one does not leak into a record. The
user's prompt still never reaches the screen.

**Text.** The Pets pane's segmented picker no longer carries a title —
in a pane that narrow it wrapped, and the track names already say what
it is.

Co-Authored-By: Claude Code <[email protected]>
The icon was a white paw on a blue gradient. Baidu's mark is a white paw
on blue: close enough that the resemblance is the first thing anyone
notices, and it is not a mark this app gets to wear.

It is now the menu bar's own motif — two dark cat paw prints walking
diagonally across a warm paper tile — drawn with CoreGraphics rather
than set in the emoji, since the glyph is Apple's artwork and has no
business being rasterised into a shipped bundle. Each print is a cat's
and not a bear's: two rounded lobes over a broad base with a notch
between them. That shape is also what keeps the silhouette away from
the logo it used to echo.

The generator keeps its structure — Apple's icon grid, the same ten
variants — so `swift Scripts/generate-icon.swift <out.iconset>` still
rebuilds the icns byte for byte.

Co-Authored-By: Claude Code <[email protected]>
The tile was paper-coloured, which was chosen to get away from the blue
the icon used to wear; orange does that job better and says "cat" while
it is at it. Amber to tangerine on the diagonal, with the prints in a
deep warm brown so they read as pads rather than as holes in the tile.

The READMEs now open with the icon, which is the only place a reader
sees the app before installing it. `Resources/AppIcon-512.png` is the
512px render from the same generator, so the mark and the app icon
cannot drift apart.

Co-Authored-By: Claude Code <[email protected]>
The icon is now the supplied artwork, masked into Apple's macOS icon
grid: the source is a full-bleed tile with its own smaller rounded
corners on a dark ground, and clipping it to the grid is what removes
that ground and keeps it the same visual weight as everything else in
the Dock. The generator no longer draws the mark — it loads the tile,
which is committed as `Resources/AppIcon-source.png`, and everything
else (the .icns, the README logo, the menu bar glyph) is derived from
it in one command.

The menu bar cannot show the picture: at 18pt it is mud, and a menu bar
icon has to be a template image — one ink colour, alpha only — so AppKit
can invert it for a dark menu bar and for the highlight when its menu is
open. So the mascot is redrawn from geometry measured off the artwork
(the head's bounding box, the two eyes, the ring's arc), reduced to what
survives at that size: the head, its two eyes as holes, and the ring as
an arc behind it. It ships as an 18pt/2x pair and falls back to the paw
emoji if the bundle has no icon, which is the case under `swift run`.

Co-Authored-By: Claude Code <[email protected]>
dncore and others added 21 commits September 15, 2026 11:52
Pi extends through a single TypeScript file in its auto-discovered
extensions directory, so the runtime writes exactly that: node built-ins
only, no package install, no settings edit, a marker at the top, and a
backup before uninstall deletes it — a file the user wrote is refused,
never touched. The extension reports lifecycle events and, at each
settle, a context reading in the same reduced shape the status-line taps
use. agent_settled, not agent_end: after agent_end Pi may still retry or
run queued follow-ups. The profile converged on exactly the events the
extension sends. Verified against a real pi session (frames captured off
the shim) plus a fake host for the tool branch, and cleanly removed and
reinstalled on this machine.
Codex's ~/.codex/hooks.json is the JSON shape Claude's configurator
already writes, so wiring is a registry entry: twelve events — including
Interrupt and PermissionRequest — merged around other tools' entries
(Otty owns four on this machine) and removed without touching them. The
profile was rewritten around the installed binary's wire structs:
Claude-shaped snake_case fields, Interrupt settling the turn, no rules
for the events Codex does not have, and a preview only when the field
is actually there. A new per-agent postConfigureHint says the one thing
the file cannot do for itself: Codex skips hooks until the user trusts
them in its own /hooks panel. Verified with codex doctor (0 warn, 0
fail) and a clean uninstall/reinstall that left Otty's entries intact.
The CLI's hooks.json loaded a probe fine, but nothing fired until sign-in
— the agent loop never starts before auth. Logged in, a labeled probe
captured every event the 1.2.3 binary actually fires, and the docs were
wrong or silent about several: tool events take matcher groups while the
rest take flat handler lists; SessionStart exists although the published
event table omits it; PostToolUse never fired in any captured turn;
Stop's terminationReason is NO_TOOL_CALL, not the documented model_stop.
One run whose hook scripts were missing proved hook failures fail open.

The integration writes one named hook (agentpet) around other named
hooks, answers each registration with the empty JSON object its contract
wants — verified harmless with a real tool call under a one-command
allowlist — and the status-line tap renders a plain replacement row,
because Antigravity's default row is visible and a command replaces it.
The rule engine grew dotted-path reads, array-first reads, and two
conditions (suppressedWhenFalseField, requiresNonEmptyField) to meet the
payload as it actually is. Fixtures are the captured frames, sanitized.
A backup's name starts with the file it came from, so `agentpet.json.…`
sorted before `settings.json.…` no matter how new it was — and the
moment a second integration backed a file up into the same directory,
the backup taken during that very call was the first one pruned and the
path the caller had just been handed was gone. Age now comes from the
filesystem, with the name only as a tiebreak inside one clock tick.

Ported from dncore#1; author CaffreySun.

Co-Authored-By: CaffreySun <[email protected]>
Co-Authored-By: Claude Code <[email protected]>
An in-process extension names the session `sessionId`; the capture
allowlist only knew the hook spelling `session_id`, so an event spooled
while the app was down replayed under a process-derived name — a second
row beside the real one. Both spellings of the session id and the tool
name are now captured; the list stays hand-written for the reason its
comment gives (a profile's summaryField is the user's own prompt).

Ported from dncore#1; author CaffreySun.

Co-Authored-By: CaffreySun <[email protected]>
Co-Authored-By: Claude Code <[email protected]>
An extension that spawns the shim from inside the agent's own runtime
writes to a pipe queued on an event loop the agent may be holding —
measured, 30 ms of a busy loop is enough for the shim's stdin deadline
to expire and the event to arrive with no session id. The shim now
reads AGENTPET_PAYLOAD_BASE64 first and falls back to stdin, so the
hook contract is untouched and an in-process reporter has nothing to
be late for.

Ported from dncore#1; author CaffreySun.

Co-Authored-By: CaffreySun <[email protected]>
Co-Authored-By: Claude Code <[email protected]>
The pipe race the Oh My Pi extension measured is Pi's race as well —
same in-process runtime, same write queued behind the agent's event
loop. The generated extension now hands the payload over in the
environment, and the template test pins both halves of that decision
(AGENTPET_PAYLOAD_BASE64 present, proc.stdin absent).

Co-Authored-By: Claude Code <[email protected]>
The lint exempts /Users/x and /Users/someone, and "/Users/dev/Documents"
as a "generic" sandbox path is neither. It has failed the check on every
push since the Antigravity tests landed — including v0.9.6 — so every PR
against main shows a red lint job that is not the contributor's fault.

Co-Authored-By: Claude Code <[email protected]>
The capture comment credited the camelCase session id to in-process
extensions, and that is wrong twice over: Grok's captured wire carries
both spellings of every core field, and the extensions we ship report
`session_id` with a camelCase `toolName`. Antigravity's wire is camelCase
throughout, which the old wording ("hooks use session_id") also missed.

The spool test was named after a wire that does not spell it that way;
the redaction path does not care which agent sent the payload, so the
envelope is agent-neutral now.

Wording fix ported from dncore#2 (1a0bad5; author CaffreySun), whose branch
was closed as superseded before this part of it landed.

Co-Authored-By: CaffreySun <[email protected]>
Co-Authored-By: Claude Code <[email protected]>
Oh My Pi is a rebranded Pi (`@oh-my-pi/pi-coding-agent`): the same extension
API, `~/.omp/agent` instead of `~/.pi/agent`. It is the first agent here
whose integration is a *file* rather than an edit — it loads every `*.ts` in
`~/.omp/agent/extensions/` at session start — so the runtime installs one
file it owns outright: snapshot, back up, write atomically, read back,
refuse to touch a file that does not carry the marker it recorded, and
delete only while the recorded `// shim:` line is still in it.

Two things about the integration are worth knowing before reading the diff.

What it reports is the extension's own event set, so the profile maps
`tool_approval_requested` to waiting (the approval gate — the counterpart of
Claude Code's permission prompt) and `tool_execution_start` carrying `ask`
to waitingInput (the `ask` tool *is* a question put to the user). `agent_end`
is dropped by the extension itself when the payload says `willContinue` — a
retry is already scheduled, so the turn has not settled. A new rule
condition, `whenToolName`, exists so the narrow rule can precede the general
one for the same event.

The payload reaches the shim in the environment, not down a pipe. The
extension runs inside the agent's own runtime, where a write to a pipe is
queued on an event loop the agent may be holding: measured, 30ms of a busy
stretch between spawning the shim and writing to it was enough for the
shim's 20ms stdin deadline to expire, and the event then arrived with no
session id at all — a row the pet could never fill. `AGENTPET_PAYLOAD_BASE64`
is handed to the process by the kernel at spawn time. The stdin path is
untouched, byte for byte, for Claude Code, Grok and Codex.

Two defects surfaced while wiring it up, both triggered by there being a
second integration in the runtime:

- Backups were pruned by file name, which is chronological only while every
  backup in the directory came from the same file: `agentpet.ts.…` sorts
  before `settings.json.…` however new it is, so the backup taken during a
  call was the first one that call deleted, and the path it had just printed
  was already gone. Pruning goes by modification date now, and the new test
  fails against the old ordering.
- The spool and event-log allowlist knew only Claude Code's `session_id` and
  `tool_name`, so a spooled Oh My Pi event lost its session: replaying it
  after a restart drew that session under a process-derived name, beside the
  real one. Both spellings are kept, and the test pins it.

Verified against omp 18.1.22 on this machine: the file is picked up at
session start (no `--extension` flag), a real session delivers
`session_start / agent_start / tool_execution_start / tool_execution_end /
agent_end / session_shutdown` with one session id and the working directory,
the app's own verbose log shows the row `Oh My Pi · 950be5 · tmp · Needs
input — ask` while a question is pending, and `--unconfigure omp` removes
exactly that file and leaves the two extensions that were already there.
471 tests.

The cost is stated in the installed file's header as well as in the docs: any
extension handler on one of the four tool-lifecycle events turns off Oh My
Pi's experimental speculative local reads
(`tools.speculativeExecution.enabled`, off by default) while the file is
installed; this file takes the two approval events, which is what makes an
approval show as "Needs input".

Not included: model, context and cost for Oh My Pi (§6.7 records why), and Pi
itself stays detect-only.
Pi and Oh My Pi extend their agent the same way — one marked file the runtime
owns, in a directory the agent scans — so they now share the generated file,
with the event set as an explicit parameter (ExtensionTemplate).

The events are the only thing that really differs, and they differ in both
directions: `agent_settled` does not exist in omp 18.1.22 (checked event by
event against the installed package), and a plain `agent_end` is not a settle
on Pi, which may still retry or run a queued follow-up after it. One file
could not be right for both, which is what the parameter encodes.

Sharing the file also shares the fixes that were in only one of them:

- The payload travels in the environment for both (AGENTPET_PAYLOAD_BASE64),
  not down a pipe — the race the Pi extension had shipped with, measured at
  30ms of busy loop.
- The context reading (ctx.getContextUsage() / ctx.model) is taken at every
  settle for both, in the one reduced shape the status-line taps use.
- The write goes through ConfigTransaction.performText for both, so the Oh My
  Pi path no longer carries its own copy of snapshot / backup / atomic write /
  read-back.

Field spelling converges on what Pi's extension already sent — `session_id`,
`cwd`, `toolName` — so each profile has one reader and the generator has one
payload builder.

Six rounds of adversarial review followed, and their fixes are folded into
this commit rather than left as a trail of "fix the review" commits: the cost
note is per event set (it had leaked Oh My Pi's paragraph into Pi's file), the
Pi half of the event-set parameter is now guarded by a test that reads the
generated file, the docs stopped describing the pre-convergence world in the
places this commit created, and the CLI labels one quantity the same way on
every surface.

Not unified yet, and worth a decision rather than a guess: the two
configurators still recognise their own file differently. Pi checks its marker
plus the recorded shim path; Oh My Pi checks the marker plus the recorded
`// shim:` line, which is strictly stronger — that line carries both the path
and the agent id. Folding Pi onto it is a one-line change to
`PiConfigurator.entriesPresent`, but it changes a rule that shipped today, so
it is proposed rather than taken.

542 tests.

The review of this commit's delta is folded in: the generated file's comment
line is comment-safe now (a path with a line separator used to end the comment
and break the file, while the record still claimed it present), the cost note
is no longer prefixed twice, `report()`'s payload build is wrapped again, the
Pi suite pins its event set instead of deriving it from the file under test,
the profile's field names are tied to both payload builders, and the docs
stopped claiming Oh My Pi awaits this branch.
You asked for this and specified how, and it is the same change the review
round was asking for from the other side: the docs claimed one backup policy
covered every edit while Pi's uninstall wrote its own copy straight into the
directory, bypassing the retention pass.

- Presence and removal now ask one question for both agents: is every line the
  record says was written still in the file. They move together, as you asked
  — a file that fails presence cannot be deleted by a different rule.
- Pi's record entry is the `// shim:` line from here on, defined once in
  `ExtensionTemplate.markerLine` and both written into the file and recorded
  verbatim. A v0.9.6 record needs no branch: its entry is the marker line, the
  old file still contains it, so it reads present and uninstalls cleanly. Both
  generations have tests.
- Pi's uninstall backs up through `ConfigTransaction.backUp`, so the retention
  policy bounds it too; `PiConfigurator` no longer keeps a backup directory of
  its own, nor the `jsStringLiteral` only its old presence check used.
- The tests that pinned the old rule were rewritten rather than dropped: a
  moved shim is drift because the entry carries the path, and that case fails
  if the rule stops reading the record (checked by breaking it).

Docs: §6.7b and §8.8 describe one rule for both agents and name what still
differs — the marker string, the refusal error, the cost note, the event set.
Counts moved to 546.

The review of this delta is folded in too: the entry `configure` records is
pinned by a test, the v0.9.6 case uses the frozen literal rather than the
constant it is meant to outlive, a case makes the retention policy visible
(the hand-rolled backup this replaced fails it), and two texts that still
described the pre-fold rule were corrected.
Two rounds in one commit: three gaze defects found while answering "does the
pet follow the mouse at all?", and the settings work that followed.

**The gaze looked the wrong way, vertically.** `LookDirection.angle` carried
Codex's *DOM* sign convention (`atan2(dx, -dy)`, y downward) while its only
caller handed it AppKit screen points, where y grows upward — so the pet
looked *down* at a pointer above it and *up* at one below, with left and right
correct, which is why it survived unnoticed. The angle now speaks the space it
is given; `LookDirectionTests` pins it with quadrant cases and the two existing
angle tests were flipped to AppKit coordinates. Measured rather than assumed:
`NSEvent.mouseLocation.y == mainScreenHeight − CGEvent.location.y` on this
machine, and a real V2 atlas's look rows were exported and looked at (000 = up,
clockwise, row 9 c4 = right).

Two more from the same read:

- The gaze was aimed from the *window's* centre, but the window grows upward to
  hold the panel, so its centre sits `panelHeight / 2` above the pet — 20pt for
  one row, 50 for four, against a 112x121pt sprite. It comes from
  `petView.spriteRect` now, and `--selftest` checks the origin with the panel
  up (verified by breaking it: the check prints the 30pt drift).
- Under reduced motion every frame collapsed to column 0, which for a *look*
  pose is not "hold still" but "look at 000": the pet alternated between
  straight up and straight down depending on which half of the screen the
  pointer was in. A `staticPose` track is exempt now — its column is a
  direction, not a point in time.

**The size is Codex's own slider.** It was three named sizes (80/112/224), a
simplification of a setting Codex ships as a continuous 80-224 slider
(`avatar-overlay-mascot-width-px`, default 112). `pet.width` is a number now
(step 4 in the settings), and a config written before it still loads: the old
`"size": "small|standard|large"` key is read through its own `LegacyKeys`
(80/112/224) and never written again. It is a second key type rather than a
case in `CodingKeys` because a coding key with no stored property behind it
does not compile — the encoder is synthesised. A real config on disk still
says `"size": "standard"` and loads as 112pt; `--diagnose` reports it.

Around it:

- The panel may now be 300% of the pet (was 200%). The default stays 200:
  raising a ceiling must not widen a panel nobody asked to widen.
- The status message's text is settable, 8-20pt, defaulting to the 10.5pt it
  has always drawn at, expressed in points **at the default pet**: the
  effective size is `setting x petWidth / 112`, so enlarging the pet enlarges
  the message (a 224pt pet draws 21pt) and the row grows with it — never below
  the 18pt the other items in the row need, because they keep their own size.
  **The width never follows the text**: it stays a percentage of the pet, and
  `--selftest` asserts both halves plus the thing that matters most, that the
  default geometry is exactly what it was (a two-row panel is still 224x181 at
  the default pet). The dumped panel at 224pt / 20pt / 300% was looked at.

Verified: 553 tests, `--selftest` at the defaults and at 224pt/20pt/300%, and
`--help`, `--version`, `--diagnose` and `--status` all still exit 0 in a
fraction of a second. The settings form itself is compile-checked only — this
machine has no Screen Recording permission, so nothing here has seen it drawn.

Docs: ARCHITECTURE §3.3 and §3.4 (the space the angle lives in, the sprite
origin, the reduced-motion exemption, and the stale "28pt deadzone" sentence
that contradicted §3.6), §6.5 (the slider, the font rule, the legacy key) and
the appendix constants; both READMEs.

Co-Authored-By: Claude Code <[email protected]>
A macOS 26 user reported that this tap's cask "uses the removed `run` DSL
method" and that their brew also mentions `caskroom/cask`. Measured, rather
than taken at face value:

- `run` is not removed. It is in Homebrew 7.0.2, in 7.0.3 (which auto-updated
  itself mid-investigation) and on `master` — and its history says the
  opposite of "legacy": the install-steps framework landed 2026-05-22 and
  `run` was *added* on 2026-07-26 ("Add constrained install commands"). So the
  only true version of that complaint is the reverse of how it was told: on a
  Homebrew older than seven weeks the whole cask fails to load, because `run`
  does not exist there yet.
- `caskroom/cask` appears nowhere in Homebrew 7.0.3; the string is from the
  pre-2020 cask era. It can only be that user's own machine, where an ancient
  tap is still installed (`brew untap caskroom/cask`).
- What *is* true and reproducible is our own install line: on a current
  Homebrew, `brew install --cask agent-pet-runtime` stops with "Refusing to
  load cask dncore/agent-pet-runtime/agent-pet-runtime from untrusted tap
  dncore/agent-pet-runtime" — third-party taps are untrusted by default now.

The step itself was doing nothing, on any install, ever. Homebrew downloads
with curl and nothing quarantines that: `Quarantine.propagate` returns early
unless the container it just unpacked already carries the attribute, and
`Quarantine.cask!` — the function `download.rb` calls at install time — is an
empty stub, in 7.0.3 and on `master` alike. So the block bought no protection
and cost a version floor of seven weeks. It is gone, with the reasoning left
in its place, and the caveats keep the `xattr -dr` line that is the real
remedy for a copy quarantined by a browser download.

The install line now names the cask in full, which is Homebrew's own way past
the trust gate — asking for `tap/name` trusts that one cask and prints so,
where the short name cannot. `brew trust dncore/agent-pet-runtime` is offered
as the whole-tap alternative; both spellings were run here, and the trust
store shows the effect.

Verified: `brew style` on the regenerated cask is clean, the diff against the
file in the tap is exactly the removed block, no step DSL remains, and the
fully qualified install auto-trusts (`==> Trusted cask …`). The tap's README
carried the same two errors and is fixed there as well.

Co-Authored-By: Claude Code <[email protected]>
Antigravity's `PreToolUse` hook must answer with a decision — `allow`, `deny`,
`ask`, or `force_ask`. There is no "no opinion"; the shim's observer answer is
`{}`, and a decision-less result is read as a denial. So the integration denied
every tool call in the agent, with an empty reason, and a user's agent stopped
working the moment the hooks were installed (their dncore#3 and dncore#4, 2026-09-17 — the
model side sees `Error invalid tool call: … (invalid_args) tool call denied with
reason:`, the user side only `Tool call denied by pre-tool hook:`, which is why
the pasted errors end at the colon). Reproduced by another project with a probe
hook that does nothing but print `{}` (crossby#147, agy 1.1.14), reported by
two more (ai-memory#352, cmux#8921 — that one on a hook timeout, so a hook that
cannot answer is fail-closed; only a hook that cannot *load* is fail-open).

- `PreToolUse` is no longer installed. `PostToolUse` stays: its contract is an
  empty object and it gates nothing. The profile still reads `PreToolUse`, so a
  session reported through a config an older version wrote still shows a tool
  name.
- A config an older version wrote loses that line on the next launch:
  `AgentConfigurator.removeObsoleteEntries` (default: nothing to do) drops
  entries this version no longer writes — only lines the runtime recorded, only
  while the file still holds exactly what was recorded, and without re-pointing
  the shim, so a development build started beside an install cannot take the
  install's hooks over. The bottom bar says what it did.
- `entriesPresent` now treats an obsolete event under our named hook as drift,
  so a migration that refuses (a human edited in there) is visible on the card
  rather than silent, and the card's wording covers both ways into that state.

The 2026-09-15 note that `{}` had been measured harmless is corrected in
ARCHITECTURE §6.7b — that test ran an allowlisted tool call, which never
reaches the decision branch — and the READMEs now carry the rule beside "the
shim always exits 0": a hook whose result can gate the agent is not a hook this
runtime registers, however harmless an empty answer looked in testing.

Verified: 557 tests in 85 suites pass; the app builds and `--selftest` passes;
`--help`/`--status`/`--diagnose` exit 0 without blocking; the CI sanitisation
lint and the shim-contract checks pass locally; and a real 7-event config from
0.10.x, replayed in an isolated home through a real launch, loses exactly the
`PreToolUse` line, keeps its shim path, reports the line in the bottom bar, and
does nothing on the second launch.

Co-Authored-By: Claude Code <[email protected]>
Two reports from the same session, both about what the pet fails to say.

**A working pet was drawn exactly like a resting one.** The gaze is folded into
the rows Codex folds it into, and `running` was one of them. A look pose is a
single *static* frame, and it replaced `idle` and `running` with the *same*
cell — so on a pet with look rows (V2; V1 has none), a working pet and a
resting one were pixel-identical for as long as the pointer was anywhere on
screen, which is always. The one state that says "it is working" was the one
that could not be seen.

`running` is dropped from that set. The layer-4 gaze call the change makes
unreachable goes with it: with `running` out, no state row can be replaced at
all, and the `settledIntoIdle` guard existed only for that case. Verified
against the live app with the real pointer on three displays —
`state=running drawing=running r7c0..c5` in a loop while an agent works, and
`state=idle drawing=look-b r10c2` the moment the activities are cleared. Same
trade as the idle timings in §3.1a: fidelity to Codex loses to the pet reading
correctly. ARCHITECTURE §3.6, both READMEs; the local notes carry the "don't
put it back without watching a long turn".

**A refused pet package vanished from every surface at once.** Discovery was
`try? … else continue`, so a folder whose `spriteVersionNumber` contradicts its
sheet's size, a manifest whose fields decode to the wrong types, or a
spritesheet that is not an image left no row in the manager, no line in the
log, and no mention in `--diagnose`. The whole symptom was "my pet is missing".
Refusing such packages is right — Codex's own TUI refuses them too — but
saying nothing is not.

- `PetLibraryScanner` (Core) owns the scanning; which directories to scan stays
  the app's business. It returns `entries` + `skipped(name/root/reason)`, and
  `--diagnose` prints the second half. A folder with no manifest at all is
  still passed over in silence: that is not a pet, and reporting it would be
  noise on every build directory. Splitting it out is also what makes it
  testable — it lived in the app target, where no test can reach it.
- `PetPackageError`, `PetManifestError` and `ImageDecodingError` each carry a
  `message`, so a refusal reads as a sentence instead of an enum case in both
  `--diagnose` and the manager. `atlasUnreadable` finally carries the `detail`
  it was throwing away — the version/dimension contradiction is the package's
  own statement of why, and it used to be discarded at the throw. A decode
  failure now names the field it happened at.
- `--verbose` gains the third leg of its story: `[pet] frame: state=… drawing=…`,
  one line per *change*. The events and the panel were already logged; what the
  pet actually draws had no observer on this machine at all, which is how a
  "the pet isn't running" report stays unfalsifiable.

Verified: 571 tests in 87 suites; the new self-test check and the scanner tests
were each shown to fail against the old behaviour before being trusted
(`gazeRows` restored, `skipped` forced empty); `--selftest` passes for a V2 and
a V1 pet; `--help`/`--status`/`--diagnose` exit 0 in under a second; the CI
shim contract, the plist lint and the sanitisation lint pass locally; and this
build was installed over /Applications and watched through a full
running → idle → running cycle with the real hooks.

Docs: ARCHITECTURE §3.6 (the gaze rows), §6.3 ("skipped" is not "silent"),
§7.1 (the version step was missing from the validation order) and §7.4 (whose
V2 row still claimed "partially supported"), plus both READMEs.

Co-Authored-By: Claude Code <[email protected]>
The two follow-ups from the last release, plus the one the second of them
surfaced on its own.

**The gaze no longer replaces the wave.** `waving` was the second and last of
the rows Codex folds a look frame into, and the same reasoning that took
`running` out applies: a look pose is one *static* frame, so folding it into a
row deletes that row's content for as long as the pointer is on screen. Here
that meant the pet's own hello — the introduction it plays once per pet, and
the click greeting — was drawn as a stare, and `Preview Animation → waving`
showed a pose instead of a wave. The gaze now lives in exactly one place, the
idle fallback, so `gazeRows` and its `overriding:` parameter are gone with it:
`lookFrame(_:_:)` answers only "what pose would the pointer ask for".

The self-test's introduction check pins a *direction* while it asserts the
wave, because that is the only condition an introduction is ever seen in —
with the gaze unpinned the check passed for the wrong reason before.

**A refused package is shown, not dropped.** The scanner's `skipped` list now
reaches the two places a user actually looks:

- the Pet Manager lists them under "Not listed" — folder name, reason, and a
  button to reveal it in the Finder — unless the list is empty, in which case
  the empty state carries them instead (that is where a missing pet is looked
  for).
- the launch alert for "no playable pet" names every refused folder and its
  reason. It used to say "Nothing in <directory>" while the user's folder sat
  in that directory, which is the single most misleading thing this app could
  say to the person it was said to.

The same string is printed on the headless path, so `--selftest` on a machine
with no playable pet reports the reasons instead of hanging on a modal.

Verified: 571 tests in 87 suites; the wave and running tests were flipped
first and shown to fail against the old rule; `--selftest` passes on a V2 pet,
a V1 pet, and the real library; the manager's "Not listed" section and the
no-playable-pet message were read back through the accessibility tree on an
isolated instance (this machine cannot screenshot); and the installed build was
driven live — `Preview Animation → waving` draws `waving r3c0..c3` and hands
back to `running r7` — with the frame log added in 0.11.3.

Docs: ARCHITECTURE §3.6 (the gaze is now one row, with both removals and why),
§7.4 (the three exits for a refusal), both READMEs.

Co-Authored-By: Claude Code <[email protected]>
At the largest text size the settings allow, the row chopped the one word the
message exists to say. On a 112pt pet at 255% width and 20pt, in a row that
also carries a task and a context bar:

    20pt      ✳ pro…    ▓▓░ 72%  Needs inpu…
    10.5pt    ✳ proje…  ▓▓░ 72%  Needs input — Claud…

— raising the size showed *less* of the status than the default did, which is
backwards.

`frames` squeezed every flexible item down to the constant floor
(`minimumItemWidth`, 34pt) and never looked at the item's own `minimumWidth`,
while the message's minimum was a scaled fraction (`17pt × textScale`) rather
than the width of the text it has to show. The message column ended at 123pt
against a 127pt "Needs input…", so AppKit did as it was told and cut the
wording mid-word.

The message now carries `preferredWidth` — the width it keeps for itself
before the squeeze starts on anything else — measured as its own label plus
the ellipsis that marks the detail as cut. Every other item keeps the 34pt
floor, so nothing else moves. The claim is all-or-nothing: it is paid only
when what remains after every other item holds its `minimumWidth` covers the
wording whole, because paying part of it buys nothing (the wording is cut
anyway) and costs the neighbours their room. The room comes out of the items
that only name things — in the case above, the task name.

- `--selftest` checks the wording at 20pt on a 112pt pet at 255%: drawn ≥
  label + ellipsis, shown failing first ("got 123pt for a 127pt wording").
  The waiting session carries a body now, as a real permission prompt does.
- The default size is untouched, by pixels: the nine-item panel at 200% and
  the 255% configuration render identically before and after (0 differing
  pixels each), and so does the extreme 100%-width/40pt case, which the
  all-or-nothing rule leaves exactly where it was.
- The panel dump prints the app's own config rather than the one the test
  last handed the view, which is not what the user has on screen.

Co-Authored-By: Claude Code <[email protected]>
…ern it

Five sizing rules from a user spec, and one changed meaning.

**Rows share one set of columns.** Every row is laid out on the same
columns, in the configured item order, each column as wide as the widest
cell that has one; a row without an item leaves the cell blank and the
columns after it do not move up. Laid out per row, a session id one
character longer shifted everything after it along that row, and the panel
read as several panels stacked. The squeeze moved to the columns too
(`columns(for:)` + `squeezed(_:into:)`), so every row gets one answer — and
a column is now drawn in all rows or in none.

**The panel fits itself to its content** (`fitsText`, on by default):
width = clamp(what the columns need, the pet, the setting's maximum). The
message column is sized for its wording plus a fixed 120pt of detail
(`baseMessageDetailAllowance`), not for the 200-character body: counting the
body would pin the panel to the maximum forever and move it with every
message; counting only the label would leave the detail nowhere to draw, and
the detail is where "which tool an approval is waiting on" lives. Never
narrower than the pet: the panel is the ground the pet stands on.

**The width does not twitch.** Content changes with events, so
`PanelWidthGovernor` (Core, tested) takes new room at once and gives it back
only after the content has stayed narrower for two seconds, clocked from the
first narrow request rather than the latest.

**`autoScale`** (off by default) hands both settings to the pet: the text
draws at the default ratio (10.5pt at 112pt, 21pt at 224pt) and the width
asks for the default 200%, whatever the sliders say — they grey out and keep
the user's values for when it is switched back.

**`alignment` now means the panel's anchor against the pet** — left puts the
panel's left edge on the pet's left edge and grows to the right, right
mirrors it, centre is what the window has always done — and not how the text
inside is set, which is always left-aligned (user decision). The sprite is
therefore no longer centred in the window: `Alignment.spriteOriginX(
inWindowWidth:petWidth:)` / `windowOriginX(forSpriteX:...)` (Core, tested)
drive `PetView.spriteRect`, the tail, hitTest and tracking, the gaze origin,
`resizeWindow` and `savePosition` — which still stores the sprite's own left
edge, so alternating anchors cannot walk the pet across the screen one quit
at a time.

Also: `--selftest` puts no windows on screen any more. It showed the pet's
window on every run, and opened the manager in front once per run, so a
dozen configurations swept in a row strobed a pet on and off the desktop and
took the focus out of whatever was being typed. Every window-presenting path
is behind `HeadlessMode` now — the render path only ever needed a window,
not a visible one, which is how it already ran on CI. The two checks that
genuinely need something on screen say they skipped instead of passing
hollow, and the sidebar check keeps the half that does not (the toggle is in
the toolbar at all). The self-test also stopped reading
`view.currentPanelConfig` as "the app's config": those restore lines restored
to the config the test had just mutated, so later checks read the wrong one.

Checks: twelve configurations pass the self-test — fit on and off, both new
switches, three anchors, pets 80–224pt, text 8–20pt — including "every row is
laid out on the same columns: 9 columns, 13 cells, each kind at one x",
"the panel fits itself to its content: 211pt for one message item, 224pt
(the maximum) with the fit off, and the pet's own 112pt for a single glyph",
and "scaling with the pet ignores the sliders: a 224pt pet draws 21pt text in
a panel of up to 448pt". 579 Core tests pass, six of them the governor's.

Co-Authored-By: Claude Code <[email protected]>
… manager

Three fixes from use.

**A squeezed row gives room up in priority order.** The flexible columns were
drained in list order, so the task always paid first: at 255% of a 112pt pet
with 12pt text it sat at 38pt for a 40pt name, and widening the panel to 300%
left it exactly there — every extra point went to the message's detail
instead. The order is now: the message's *detail* (the preview under its
wording, the one thing in the panel that is a bonus rather than content),
then the columns that only name things (session id, model, tool, usage
figure, cost, limits), then the task's name, and last the message's own
wording, which the all-or-nothing floor still refuses to give up until the
row truly cannot pay. Measured: "project" now survives at 255%, and the 300%
panel spends its extra room on the detail ("— Claude wan…").

**The manager window is 900x680 by default, 760x520 minimum, and remembers
its size.** 860x580 put the Pets page's "Use on Desktop" and "Reveal in
Finder" below the fold — a 240pt preview, a track picker, the description,
the grid and the buttons come to more than that — so the page looked
complete and was not. The size is stored as the content size in
`manager.window.size`, clamped to the minimum when read, and written in
`windowDidResize` as well as at close: the manager is often open when the app
quits, and `windowWillClose` never runs then.

**The Pets page's highlight lives in the model.** It was `@State`, so
switching tabs and coming back dropped it and re-opened on nothing.
`AgentPetModel.highlightedPetID` holds it now, which also gives the page its
default: the first time it is opened in a launch it starts from
`currentPetID`, the pet actually on the desktop. Memory only, on purpose — a
relaunch should show what is running, not what was last browsed.

Checks: the new self-test checks were shown failing first — the task check
against the old order reports "got 38pt for a 40pt name" — and ten
configurations pass (fit on and off, both anchors, pets 80–224pt, text
8–20pt), including "the manager opens at 900x680 and will not go below
760x520", "a size it is dragged to is remembered", and "a squeezed panel
keeps the task's whole name". 579 Core tests pass. The self-test reads and
restores the machine's own manager size around its own check, so running it
does not change it.

Co-Authored-By: Claude Code <[email protected]>
The app menu gains `Settings…` on ⌘,, which puts the manager on its Settings
section — opening it if it is closed, re-fronting it if it is not.

In the main menu, and only there. The status menu deliberately carries no key
equivalents (`makeMenu`): an accessory app with a nonactivating pet panel is
never the active one, so a shortcut there would never fire while advertising
itself. The main menu is displayed exactly when the app *is* frontmost, which
for this app means exactly when the manager window is open — so the one thing
left for ⌘, is the case it is for: already in the manager, wanting Settings.
A global hotkey would reach the pet on the desktop, and would take ⌘, away from
every other app on the machine; not doing that.

**The press used to take about a second** (user report, 2026-09-18). Layered
timing found three synchronous main-thread costs stacked in it:

- `openManager()` → `refreshAll()` → `refreshAgents()` spawns one `--version`
  process per registered agent: 445–705ms measured.
- `MainWindowView.onAppear` called `refreshAll()` again, so opening the manager
  paid for agent detection twice.
- `show()` rebuilt the window's whole view tree — SwiftUI tree, hosting
  controller, thumbnail decode — for a window that was already on screen:
  200–357ms.

None of it is needed to switch a section, and because it all runs before the
window can repaint, the section appeared only after it finished.

Now: `show()` rebuilds a fresh tree only for a window that is not on screen,
which is the case the rebuild exists for (a closed window stops receiving
SwiftUI updates); the view no longer refreshes on appearing, so one open costs
one refresh, in the code that opens the window; and `openSettings()` re-fronts
an open manager rather than re-opening it. Measured: 0–2ms for the press,
against 652ms for the same press before.

**A diagnostic run counts as open.** `--selftest` and friends never put the
manager on screen (`presents = !HeadlessMode.isActive`, so that checking the
window does not take the user out of whatever they are typing), which makes
"on screen" permanently false there. The guard therefore reads a window that
exists in `HeadlessMode` as open: there is no screen to keep alive, and
treating it as closed would rebuild the tree between the two presses the
shortcut's own check makes — measuring a rebuild no user can cause.

**The self-test covers the shortcut, through the menu.**
`checkSettingsShortcut` hands a real key event to the app's own main menu —
`NSApplication.sendEvent` first, `NSMenu.performKeyEquivalent(with:)` only if
that delivers nothing, with the route and the action's duration printed rather
than assumed — and checks that the item is offered on ⌘ alone; that a closed
manager opens on Settings; that an open one moves the same window there
*without* rebuilding its view tree (`contentViewController` identity) and
without holding the main thread for more than 250ms; and that a focused
Settings field does not eat a menu shortcut (proved with the window's other
shortcut, ⌃⌘S, since the sidebar is observable where the section is not).

The check follows the window by identity rather than by title. A closed manager
window stays in `NSApp.windows` (`isReleasedWhenClosed = false`) and a
diagnostic run puts none of them on screen, so "first window with this title"
can be the previous check's — the sidebar check opens one of its own.

Four negative controls, each run and reverted: pointing the item at
`openManager` fails two assertions; deleting the item reports that no ⌘, item
exists; putting the rebuild back reports a 357ms rebuild; and making the layout
ignore `row.alignment` turns the focus assertion's scaffold into a failure.

What this does not do: opening the manager from closed still pays the agent
detection (445–705ms) and the window's first build — moving detection off the
main thread is its own change — and ⌘, with the manager minimized is
unverified, so the self-test prints `– not checked` for it rather than guessing.

Verified: 579 tests in 88 suites; the CI gate set run locally (`swift build
--build-tests`, `swift test --parallel`, the shim contract, the bundle and
plist lint, `--selftest`, `--diagnose`/`--status`/`--help` exiting 0, and the
leak scan); `--selftest` passes on the assembled bundle and on the copy
installed into /Applications; the press was measured both ways (0–2ms now,
652ms before) through the self-test's own numbers.

Docs: ARCHITECTURE §6.5e (the decision, the timing, the assertions and their
counter-evidence, and the diagnostic-run nuance), both READMEs (status tables).

Co-Authored-By: Claude Code <[email protected]>
@dncore

dncore commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Rebased onto main (which has moved under it — the window-size fix touches the same show()) and pushed as the branch pr-5-rebased, two commits: yours, then one change from the review.

The change: isOpen now also tests isClosed — set by windowWillClose, cleared when show() brings the window back. HeadlessMode alone made a closed window count as open in a diagnostic run, so show() never rebuilt there and the close-and-reopen path a user takes was unreachable from --selftest; the window-size check beside it (reopening it keeps the size it was left at, added 2026-09-18) would have gone green without ever reaching the swap it is about. Both checks keep their meaning now — your second ⌘, press still measures no rebuild (0ms), and the size check closes the window first, sees the tree swapped, and keeps its size. Shown by removing the flag: ✗ reopening a closed manager did not rebuild its view tree, with the size line still reporting success.

Verified on the rebased branch: 579 tests in 88 suites, --selftest green (the four shortcut assertions and the manager ones), your timings reproduce (394–408ms from closed, 0ms from open). §6.5e gained a sentence for the closed-window nuance.

Happy to merge it as it stands, or take changes first — your call.

@CaffreySun

Copy link
Copy Markdown
Contributor Author

Read it, reproduced it, and agree — isClosed is the right call. My predicate was wrong in a way I couldn't see from here: on e48e67a (my commit + new main, without your change), --selftest fails exactly the assertion you added,

✗ reopening a closed manager did not rebuild its view tree

while the size line beside it reports success — the false green you described, in the flesh. "Nothing is on screen in a diagnostic run, so the distinction is meaningless" was the wrong conclusion: it made the one path the rebuild exists for unreachable from the check that is supposed to watch it. Thanks for catching it.

Your branch is not green on this machine, though (macOS 26 / Darwin 25.6), and it is deterministic — three runs, same line, while e48e67a passes the same check:

2daa3a2  ✓ reopening it keeps the size it was left at (812x596)
         ✗ the window has no toolbar item to toggle the sidebar with
e48e67a  ✓ the toggle lives in the toolbar — position not checked …

It is not a missing toolbar. Dumping the items either side of the close, same window, same run:

before the close   items=1  [ <uuid>, view set ]
after the reopen   items=4  [ NSToolbarFlexibleSpaceItem                  (view nil)
                               com.apple.SwiftUI.navigationSplitView.toggleSidebar
                                   "Hide Sidebar"                        (view set)
                               splitViewSeparator-0                      (view nil)
                               <uuid>, view set ]

The sidebar check finds the toggle with window.toolbar?.items.first?.view. On the rebuilt tree that is the flexible space, so the lookup returns nil and the check fails on an item that is present and has a view. Two things were latent here: the toolbar a window starts with holds one item that is not the toggle either, so items.first was never really "the toggle" — and a rebuilt tree's toolbar is populated properly (the rebuild path lays the window out on its way through, setFrame), which is what puts something in front of it. Your isClosed commit is what made that shape reachable; the positional guess is what it landed on. layoutSubtreeIfNeeded() on the content view, from the check or from show(), does not change the layout — I tried both.

Fix pushed as fix/sidebar-toggle-lookup on my fork — one commit on top of pr-5-rebased, replacing the positional lookup with SwiftUI's own item identifier (…navigationSplitView.toggleSidebar), with the measurements above in its body:

git fetch https://github.com/CaffreySun/agent-pet-runtime fix/sidebar-toggle-lookup
aceb0ee  Name the sidebar toggle instead of trusting its place in the toolbar

Verified on top of your branch: --selftest green end to end (the four shortcut assertions, the reopen and size assertions, the sidebar ones), 579 tests in 88 suites, and the guard still fires — pointing the lookup at an identifier that does not exist prints ✗ the window has no toolbar item to toggle the sidebar with. Since your green run may be a different macOS, the item layout could be version-dependent, which is one more reason not to name the item by position.

One thing I measured and did not change: on this SDK, deleting MainWindowView's .toolbar { … } no longer removes the toggle item at all — SwiftUI installs toggleSidebar for a NavigationSplitView by itself — so the existence half of that check cannot catch that regression here any more. What caught it was the size assertion beside it (the window came back 812x628, the toolbar's height, instead of 812x596). Your call whether that is worth its own change; I left it alone rather than fold a second decision into this one.

So: take the fix (or merge pr-5-rebased as it stands and I will follow up) — either way, once it is in main I will close #5 as superseded by the merge, rather than leave a branch three commits behind sitting on it.

@CaffreySun

Copy link
Copy Markdown
Contributor Author

Noted the close — leaving #5 where you put it. pr-5-rebased is the version that carries this, so nothing is lost from my side, and I can see it rebased onto the new main.

One thing it does not carry, and it is the one thing --selftest fails on here (macOS 26, two runs, both times the same line):

✓ reopening it keeps the size it was left at (812x596)
✗ the window has no toolbar item to toggle the sidebar with

That is the positional toolbar lookup from my comment above — items.first is a flexible space on the tree a reopen builds. I re-verified against the branch as it stands now (d77e8c9), and the fix rebased onto it is green end to end, 579 tests included:

git fetch https://github.com/CaffreySun/agent-pet-runtime fix/sidebar-toggle-lookup
dd6fedb  Name the sidebar toggle instead of trusting its place in the toolbar

If pr-5-rebased is what goes into main, take that commit with it — cherry-pick or merge, whichever suits your history. I would rather you knew it is red on this SDK before it lands, since the workflow only warns on --selftest; if your run is green, the lookup is still naming the item by position rather than by identity.

And one question so I know whether to keep the branch around: is pr-5-rebased the vehicle, or has the shortcut been dropped?

@CaffreySun

Copy link
Copy Markdown
Contributor Author

Accepted — d77e8c9 is the right call and I reproduced the reason for it independently: with !isClosed taken back out (my predicate, e48e67a), --selftest fails exactly the assertion it adds — ✗ reopening a closed manager did not rebuild its view tree — while the size line beside it still reports success. My "nothing is on screen in a diagnostic run, so the distinction is meaningless" made the one path the rebuild exists for unreachable from the check that watches it. No objection to anything in it, including the §6.5e sentence.

Step 2, their script, clean worktree, isolated home (git worktree add at d77e8c9, so nothing of mine is in the tree):

swift build --build-tests        Build complete! (13.50s)
swift test --parallel            Test run with 579 tests in 88 suites passed
./Scripts/build-app.sh           build/AgentPet.app assembled (6.7M)
CFFIXED_USER_HOME=$ISO CODEX_HOME=$ISO/.codex … --selftest

Both groups you asked for are there:

Settings shortcut
  ✓ the main menu offers “Settings…” on ⌘,
  ✓ from a closed manager, ⌘, opens it on Settings — title bar included (action 328ms)
  ✓ from Activity, ⌘, moves the same window to Settings, without touching the view tree (action 0ms)
  ✓ with a settings field focused, a menu shortcut still fires (⌃⌘S flipped the sidebar)
Manager sidebar
  ✓ reopening it keeps the size it was left at (812x596)

But it is not PASS on macOS 26 — one line, on the branch as pushed, in the same isolated run:

  ✗ the window has no toolbar item to toggle the sidebar with
FAIL (1 problem(s))

That is the positional toolbar lookup from my last two comments: after the reopen rebuilds the tree, items.first is NSToolbarFlexibleSpaceItem (no view) and the toggle is second (com.apple.SwiftUI.navigationSplitView.toggleSidebar, view set). Same script, one commit on top (fix/sidebar-toggle-lookup, dd6fedb, +12/−1):

  ✓ reopening it keeps the size it was left at (812x596)
  ✓ the toggle lives in the toolbar — position not checked: a diagnostic run puts
    no windows on screen, and SwiftUI does not lay out one it cannot be seen in
Settings shortcut
  ✓ from a closed manager, ⌘, opens it on Settings — title bar included (action 317ms)
  ✓ from Activity, ⌘, moves the same window to Settings, without touching the view tree (action 0ms)
  ✓ with a settings field focused, a menu shortcut still fires (⌃⌘S flipped the sidebar)
PASS
git fetch https://github.com/CaffreySun/agent-pet-runtime fix/sidebar-toggle-lookup   # dd6fedb

Its own negative control: pointing the lookup at an identifier that does not exist prints ✗ the window has no toolbar item to toggle the sidebar with, so the guard still fires. Take it or leave it — it is one commit, on top of your branch, nothing else in it.

Step 3(a) done: pr/settings-shortcut on my fork was force-updated to your d77e8c9 (was e8e9e40, the stale a752667 base). One catch: #5 is CLOSED, so the updated branch does not make it mergeable by itself — it needs a reopen if you want to merge it from here, otherwise merge pr-5-rebased directly (and take dd6fedb with it if you want that line green).

On the count: 579 tests in 88 suites, three runs — same 88 suites as you report, four fewer cases. I could not find anything environment-gated that would explain it (the three enabled(if:) suites all run here: the shim exists and ~/.codex/pets has 3 packages; the only parameterized argument lists are fixed tables like AgentState.allCases). If you tell me which four you see, I will dig — nothing failed in either run.

@CaffreySun

Copy link
Copy Markdown
Contributor Author

Branch is now mergeable — rebased onto current main, so it is a fast-forward: main (11e889c) is an ancestor of the head, nothing to conflict with.

fork  pr/settings-shortcut = 59fdcb2
      59fdcb2  Name the sidebar toggle instead of trusting its place in the toolbar   ← new, third
      83a60c1  Let a closed manager count as closed in a diagnostic run                ← your d77e8c9
      dbc9d8e  Give the manager a Settings shortcut, and take the second out of it     ← my e39bf87
      11e889c  (current main)

git range-diff on the first two against your pr-5-rebased: both =, i.e. patch-identical to what you reviewed — the rebase moved them, it did not touch them. (Note it had to move: your branch sat on e5479cf, and main was rewritten again after that.)

The third commit is the one thing I would like you to look at, and it is why the branch is not the two commits you pushed. Same script you gave me, isolated home, on current main:

with your two commits only:
  ✓ reopening it keeps the size it was left at (812x596)
  ✗ the window has no toolbar item to toggle the sidebar with        FAIL (1 problem)

with the third commit (dd6fedb, +12/−1):
  ✓ reopening it keeps the size it was left at (812x596)
  ✓ the toggle lives in the toolbar — position not checked: a diagnostic run puts
    no windows on screen, and SwiftUI does not lay out one it cannot be seen in
  ✓ the main menu offers “Settings…” on ⌘,
  ✓ from a closed manager, ⌘, opens it on Settings — title bar included (action 312ms)
  ✓ from Activity, ⌘, moves the same window to Settings, without touching the view tree (action 1ms)
  ✓ with a settings field focused, a menu shortcut still fires (⌃⌘S flipped the sidebar)
  PASS

It changes only the sidebar check's lookup — items.first is a flexible space on the tree the reopen builds (NSToolbarFlexibleSpaceItem, no view), the toggle is second (…navigationSplitView.toggleSidebar, view set) — and it names the item instead. Negative control: an identifier that does not exist still prints ✗ the window has no toolbar item to toggle the sidebar with. swift test --parallel on this tree: 579 → 583 tests in 88 suites pass.

That also answers the count question I asked earlier: 583 is right, and my 579 was the branch's older base — current main carries the four extra tests. Sorry for the detour; nothing was failing either way.

If you would rather merge exactly your two commits, drop the third with git revert --no-edit 59fdcb2 or cherry-pick 83a60c1 + dbc9d8e — no hard feelings, it is 12 lines and your call. Either way, #5 needs a reopen from you (I tried; an author cannot reopen a PR someone else closed), or just merge the branch.

@dncore

dncore commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Reopened — and the close was mine, indirectly: force-pushing main (the history rewrite) closed this PR automatically, base_ref_force_pushed and closed landing in the same second with no human on either side. Nothing on your side caused it.

Your third commit is reviewed and accepted. I verified it here: swift build --build-tests and swift test --parallel (583 in 88) on your branch, and --selftest twice in an isolated home — PASS both times, with the toggle line green. One honest note: with the old items.first lookup restored I could not reproduce your ✗ the window has no toolbar item to toggle the sidebar with on this machine (two runs, both green) — so the shape you measured (1 item before the close, 4 after, the toggle second) is yours, not something I can confirm here. It does not change the verdict: naming the item cannot pass by accident on whatever happens to be first, which is what the old lookup could, and it works on macOS 15.7.9 here as well as on your 26. range-diff of the first two commits against pr-5-rebased is = for both.

Merging now.

@dncore

dncore commented Sep 21, 2026

Copy link
Copy Markdown
Owner

The work is in main — three commits, landed directly, because GitHub will not let this PR be reopened: state cannot be changed. The pr/settings-shortcut branch was force-pushed or recreated. (422). That is also why it closed in the first place: force-pushing main (the history rewrite) fired base_ref_force_pushed and GitHub closed the PR in the same second — nobody on your side did anything. So this page can never show "Merged"; the record is here instead.

On main now, rebased onto its tip (range-diff against your 59fdcb2 is = for all three):

  • c291728 Give the manager a Settings shortcut, and take the second out of it
  • 9acac52 Let a closed manager count as closed in a diagnostic run
  • 4aa6b79 Name the sidebar toggle instead of trusting its place in the toolbar

Verified after landing: 583 tests in 88 suites, --selftest PASS (the four shortcut assertions, reopening it keeps the size it was left at (812x596), and the toggle line), CI green on 4aa6b79. Your toolbar fix is accepted as written — and note it exists because isClosed made the reopen path reachable in a diagnostic run for the first time, which is exactly the kind of thing that change was for.

You can delete pr/settings-shortcut in the fork now if you like; main holds it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants