Skip to content

fix(skills): stop tracking generated physical-skill shims (unbreaks git pull / innate update) - #591

Merged
theo-michel merged 4 commits into
mainfrom
untracked-files-merge-error-d0434c
Aug 6, 2026
Merged

fix(skills): stop tracking generated physical-skill shims (unbreaks git pull / innate update)#591
theo-michel merged 4 commits into
mainfrom
untracked-files-merge-error-d0434c

Conversation

@theo-michel

@theo-michel theo-michel commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

The failure

Updating a robot can abort before anything is applied:

error: The following untracked working tree files would be overwritten by merge:
	workspace/innate_skills/pick_socks/__init__.py
	workspace/innate_skills/wave/__init__.py
Please move or remove them before you merge.
Aborting

Root cause

#542 added the physical-skill __init__.py shims as tracked files, and shipped the code
that generates those same files at runtimephysical_refs.write_dir_shims() writes a
shim into every recording folder on every roster publish.

That gives each path two writers: git and the running robot. Whenever a robot's brain code
runs ahead of its checkout, the catalog writes the shims as untracked files, and the next
git pull / innate update refuses to check out the now-tracked versions sitting on top of
them. It aborts with the error above; innate update surfaces it only as
Git checkout failed. Fix conflicts and try again.

This isn't specific to #542 — any future release that changes committed shim content
reproduces it. It also drove a chain of complications that exist only because the
generated files are committed: quoting picked to survive ruff format so CI wouldn't fail
on robot-written content, collision renames that "rewrite a committed shim", numbered
fallback names that can rewrite a committed file when an unrelated skill is added, and
pruning that holds back so it doesn't "dirty git". #589 was already a patch in this area.

Fix

Make the runtime the sole owner: untrack the two committed shims and gitignore
*/__init__.py (plus the writer's __init__.py.*.tmp) under workspace/innate_skills/.
Comments premised on the shims being committed are updated to match — in physical_refs.py
including the ruff-quoting note, which no longer constrains the template, and in
physical.py, whose not-fetched-yet rationale named the shim as shipping in git.

No behavior change to the generator. It already content-compares before writing, refuses to
overwrite a hand-written __init__.py (marker check), writes atomically, and never raises.
A deliberately hand-written shim can still ship with git add -f.

One thing does have to move. Untracking makes the shims runtime-only, so a shipped agent's
from innate_skills.wave import Wave (demo_agent.py) now depends on the catalog having
written them — and the one update that deletes the previously-tracked copies opens exactly
that window. initialize_agents already backstops the sibling case (it writes
physical_skills/ before importing agents, for from physical_skills import X); this adds
the same guard for the folder spelling. It recreates only missing shims, scanning
metadata.json on disk rather than the roster, so it also covers the skills server still
booting past brain_client's 60s wait. Create-only: existing shims are never rewritten or
pruned there, so the skills server keeps sole ownership of content and the two writers
can't disagree.

The fix is retroactive: once the target tree no longer contains those paths, git stops
caring about the untracked copies on disk, so robots already stuck un-stick on their next
update
with no manual rm and no updater special-casing.

Verification

Simulated all three fleet states against a scratch repo with the real
X (pre-#542) → Y (#542) → Z (this fix) history:

Robot state Result
Stuck: old checkout + runtime-written shims git pull succeeds, shims on disk untouched
Already past #542 with tracked shims git checkout -B main <tag> (the updater's path) succeeds, deletes tracked copies; after the catalog regenerates them git status is clean
Normal lockstep robot clean pull; shims appear once the brain starts

An earlier revision of this description claimed nothing imports
innate_skills.pick_socks / innate_skills.wave statically. That was wrong:
workspace/innate_agents/demo_agent.py does, at module top level, and it ships tracked.
Hence the initialize_agents backstop above — smoke-tested against a scratch workspace
(creates the missing shims, no-op on re-run, leaves a hand-written __init__.py and
non-recording code subpackages untouched).

ruff check / ruff format pass on the edited files.

The */__init__.py pattern also catches a hand-written __init__.py in a code subpackage
(arm/, chess/, ...) — kept deliberately, since gitignore can't tell a recording folder
from a code one and per-folder lines would silently re-break on the next committed
recording. The .gitignore comment says so, and git add -f is the escape hatch either way.

Note that untracking removes the one thing that pinned the rendered shim: template drift
used to surface as a git diff on the committed copies at review time, and physical_refs
has no tests. Nothing catches that now.

Noticed, not fixed here

innate update auto-stashes local changes before checkout and never pops the stash
(_do_git_update, scripts/innate:2425), so a user's edits to tracked files silently vanish
on every update, recoverable only via git stash list. Same two-writer smell applies to
workspace/innate_skills/*/metadata.json, which is tracked but runtime-edited. Both belong
in a separate PR.

The dir shims written by physical_refs.py were also committed (PR #542),
giving each path two writers: the running robot and git. Any robot whose
brain code ran ahead of its checkout wrote the shims as untracked files,
and the next git pull / innate update then aborted with 'untracked working
tree files would be overwritten by merge'.

Make the runtime the only owner: untrack the two committed shims and
gitignore */__init__.py under workspace/innate_skills. Robots already
stuck un-stick on their next pull (the target tree no longer contains the
colliding paths); robots that pulled the tracked copies have them deleted
at checkout and regenerated on the next roster publish, with git staying
clean since the paths are now ignored.
@greptile-apps

greptile-apps Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes generated physical-skill directory shims runtime-owned.

  • Adds startup recovery for missing recording-folder shims before agent imports.
  • Ignores generated shim files and their temporary files under workspace/innate_skills/.
  • Updates comments describing recording-folder and shim ownership.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
ros2_ws/src/brain/brain_client/brain_client/agents/initializer.py Adds a create-only startup scan that restores missing physical-skill directory shims before agent discovery.
ros2_ws/src/brain/brain_client/brain_client/skills/physical.py Updates documentation for recording folders now that generated shims are no longer shipped.
ros2_ws/src/brain/brain_client/brain_client/skills/physical_refs.py Clarifies that generated directory shims must remain runtime-owned and untracked.
workspace/innate_skills/.gitignore Ignores generated recording-folder shims and atomic-write temporary files.

Reviews (4): Last reviewed commit: "Merge origin/main (#590 import-based age..." | Re-trigger Greptile

@theo-michel

Copy link
Copy Markdown
Contributor Author

Relationship to #589 (merged ~2h before this)

Worth stating explicitly, since #589 reasoned from the premise that these shims are
committed and a reviewer will reasonably ask whether this undoes it.

It doesn't. #589's behavioral fix — a name collision re-claims the loser as
WaveInnateOs and rewrites the shim instead of deleting it — is correct independent of
tracking, because deleting a live skill's shim breaks
from innate_skills.wave import Wave whether or not git knows about the file. That logic is
untouched here. What this PR removes is only the other half of #589's stated harm —
"dirtying git" — and the closing constraint that "the alias spelling survives ruff format
untouched, which matters since these shims are committed and CI formats them." Neither
applies once the files aren't tracked, so the corresponding comments are updated rather than
the behavior.

The consistency argument: workspace/physical_skills/ — the package these shims import
from, and the definition site for every generated class — is already gitignored
(.gitignore:97, zero tracked files) and regenerated on every roster publish. So in a fresh
clone from physical_skills import Wave does not resolve anyway; committing the folder shim
buys nothing, because the import chain is already broken at its first hop. Tracking one half
of a two-file generated system and ignoring the other is what produced this bug. This PR
makes both halves runtime-owned, which is what write_refs has assumed all along:

the whole directory is generated (and gitignored), so any .py we didn't just render is a
stale ref for a deleted or renamed skill

Practical note for anyone stuck right now: don't rm the colliding shims and pull — on
a running robot the roster publish rewrites them within seconds and the pull can lose that
race. Stage them instead, so the blob is already what the merge wants:

git add workspace/innate_skills/*/__init__.py && git pull

(Safe only while the on-disk content matches the incoming blob — check with
git rev-parse origin/main:<path> against git hash-object <path>.) Once this merges the
workaround is moot: the target tree stops containing the paths, so there is nothing to
collide with.

theo-michel added a commit that referenced this pull request Aug 1, 2026
The two changes are independent — #591 untracks workspace/innate_skills/{pick_socks,wave}/__init__.py
and gitignores the generated shims; #590 rewrites agent discovery. No file overlap, and no agent
imports the untracked shims statically (they import code skills, which are unaffected).
theo-michel added a commit that referenced this pull request Aug 1, 2026
…kages

An agent importing a physical skill the pack-local way
(`from innate_skills.wave import Wave`) breaks when the recording folder
has no __init__.py shim yet — and stayed broken for the life of the
process: the folder imports as an empty namespace package (__file__ is
None), and evict_modules_under skipped namespace packages, so no reload
could ever replace the cached module even after the catalog wrote the
shim. With the shims now runtime-generated only (untracked by #591), a
fresh workspace hits this whenever agents import before the skills
server's first publish — on a Jetson first boot the brain client's 60s
roster wait can expire while torch/Nav2 are still importing, and
demo_agent was then rostered broken until a process restart.

Two halves, either sufficient for its own case:

- evict_modules_under evicts namespace packages too, matching them by
  __path__ against the given roots. Deepest names are examined first:
  materializing a namespace __path__ re-derives it from the parent's
  sys.modules entry, which must still be cached when the child is read.
- initialize_agents writes the recording-folder shims (not just the
  physical_skills package) before the first agent import, from the
  roster's `directory` field — under the same gap-fill guard as the
  package, so the skills server stays the authoritative writer and the
  two processes can't fight over content.

Verified in the sim image: all 7 shipped agents (demo_agent included)
load on the first pass of a fresh workspace with a roster; and in the
overrun sequence (boot with no roster -> namespace poison -> server
writes shims -> reload) the reload now recovers demo_agent instead of
staying broken forever.
theo-michel added a commit that referenced this pull request Aug 1, 2026
… pruning

- hot_reload: an empty active-skill set is a deliberate everything-off
  choice, not "unset" — don't re-widen it on agent reload; and re-register
  whenever the roster was swapped, even to empty (all agents broken).
- initializer: the ordering-gap fallback now writes the recording-folder
  shims too (they're gitignored since #591), so
  `from innate_skills.wave import Wave` resolves on a fresh workspace
  before the skills server's first write.
- initializer: key broken-module rows like class rows
  (innate_agents.foo -> foo) so a row doesn't rename when a syntax error
  becomes a class-level failure; full module name on collision.
- loader: probe uses_gaze() too (gaze_control calls it unguarded); dedupe
  the registry-pruning walk into workspace_import.live_registered_classes
  (include_abstract is the one policy difference).
- brain_client_node: per-directive try/except in _svc_get_directives so a
  post-load failure can't blank the roster for every agent.
- workspace_import: extract tracebacks with lookup_lines=False (only
  filename/lineno are used — no source-file reads per broken module).
- agentPanel: copy feedback reflects the actual copy result (✓/✗), and the
  flash timer is tracked, cleared on destroy, and can't be erased early by
  a roster poll.
…ring

Review follow-ups for the untracking change:

- initialize_agents now recreates missing recording-folder shims from disk
  (metadata.json marks the folders — no roster needed), closing the window
  where a tracked agent's `from innate_skills.wave import Wave` fails after
  the update that deletes the once-tracked shims: brain_client's watcher only
  covers agent dirs, so an agent broken at boot stayed broken until a manual
  reload. Create-only — the skills server keeps sole ownership of updates
  and pruning, so the two writers can't fight over content.
- test_physical_refs.py pins the rendered shim (exact content, marker prefix,
  collision alias, removal marker) — the committed copies were the only thing
  pinning the template, and they're gone.
- gitignore also covers the atomic-write __init__.py.*.tmp leftover, and its
  comment now names the code-subpackage catch explicitly.
- physical.py's ship-in-git comment updated to the new ownership; the
  identifier-safety rationale for unquoted {name} restored in physical_refs.
Reverts the test_physical_refs.py added in the previous commit and cuts the
comments there back to the load-bearing facts.
#590 landed the untracking itself and a roster-based shim write under the
fresh-workspace guard; the disk-scan _ensure_dir_shims supersedes the latter
(works with no roster, create-only), so _regenerate_physical_refs goes back
to refs alone. Comment/gitignore hunks keep this branch's trimmed versions.
@theo-michel
theo-michel merged commit a09a98a into main Aug 6, 2026
7 checks passed
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