pilotctl: self-heal the next-steps cache so the installed fleet gets graphs#399
pilotctl: self-heal the next-steps cache so the installed fleet gets graphs#399Alexgodoroja wants to merge 6 commits into
Conversation
…fleet) The graph was cached only at install, so the feature reached only apps installed AFTER their graph shipped — the existing fleet, and any republished graph, never got hints without a manual `install --force`. That is a blocker: a feature that doesn't reach installed apps isn't shipped. ensureNextStepsFresh closes the gap from the call path, off the render: - steady state is a single stat of a freshness marker and returns instantly — the hot-path-is-a-local-read guarantee holds for all but the rare re-check; - when the 12h TTL has elapsed it spends ONE bounded (400ms) catalogue fetch, so an app installed before its graph existed renders on its very NEXT call, and a republished graph propagates within the TTL; - a fetch that can't reach the catalogue drops a short retry-cooldown marker so an offline host backs off instead of re-fetching every call; - an app the catalogue no longer publishes a graph for has its stale cache dropped. Proven live against the production catalogue: an app with no cache rendered on its first call (no --force), and the next call did zero network. Marker gating, failure cooldown, disabled no-op, and traversal-refusal are unit-tested; gosec clean (writes/removes re-confined to the install root via resolveUnder).
Completes the "keep all apps current" story on top of the metadata self-heal.
outdated/upgrade now detect a SAME-VERSION republish, not just a version bump:
install records the catalogue bundle sha to $APP/.bundle-sha256, and findOutdated
flags an app "rebuilt" when the catalogue's current bundle sha differs from the
recorded one at an unchanged app_version — exactly the aegis argv-fix shape, which
a version compare silently misses. A pre-feature install with no recorded sha
degrades to version-only detection (behaves as before). Logic split into the pure
outdatedReason() and unit-tested.
The updater keeps the app ADAPTERS current too, not just the pilot binaries: on
the same cadence and behind the same enable switch, it runs
`pilotctl appstore upgrade --all`, so a new version or a republish reaches the
fleet with no manual upgrade. Each upgrade re-runs the full catalogue-signature +
manifest-signature + trust-anchor gate, so this is automation, not new trust.
Opt out with PILOT_UPDATER_NO_APP_UPGRADE; gated by the same {"enabled":bool}
control file as binary updates (fail-safe off), unit-tested.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The new .bundle-sha256 read in scanInstalledApps uses the identical install-root + direntry + constant-filename pattern as the manifest read right above it (a pre-existing accepted finding). Annotate it so the PR introduces no new code-scanning alert; the read is confined by construction.
The installer aborted under set -e before the skill-injection first-pass on exactly the hosts agents run on: with systemctl present but systemd not PID 1 (containers/WSL/CI), 'sudo systemctl daemon-reload' returned non-zero and killed the script ~200 lines before 'pilotctl skills check'. Fresh installs were left with binaries + config but no service, no skill, and a non-zero exit. - Gate the systemd block on a booted system ([ -d /run/systemd/system ]), not just the systemctl binary, and guard daemon-reload/enable with || true so a degraded systemd can never abort the install. - Add a non-systemd Linux fallthrough that prints the portable start path (pilotctl daemon start) instead of leaving the agent with no daemon. - Correct per-tool skill paths (skills/pilot-protocol -> skills/pilotctl, the real entrypoint), fix PicoClaw heartbeat file (HEARTBEAT.md), and list Goose.
'pilotctl skills' (status) and 'skills paths' ran ForceTick, which WROTE files and then reported their pre-write state — so the first status both created the skill files and labelled them 'absent - next: create', a false failure an agent reads as a broken install. Switch the display surfaces (status, paths, and the info summary helpers) to skillinject.Plan(), the read-only dry run built for exactly this. Mutating actions (check, enable) keep ForceTick. Also correct the 'supported tools' list to match the injection manifest: add PicoClaw and Goose (real targets), drop Cursor (never a target). Adds a regression test pinning the list. Plan() lands in skillinject after v0.2.3; pin main HEAD via pseudo-version so this builds today. Go-live can repin to a tagged skillinject release.
…ough The post-install hint was four lines (PATH, daemon start, info, ping) and never showed an agent the two things it actually needs on a fresh node: the send-with --wait / read-newest-inbox idiom, and the three ways to get value (pilot-director for live data, list-agents for known specialists, appstore for local capabilities), plus the peers/trust half. Replace it with a numbered GET STARTED block covering all of it, with copy-paste examples. Ends by pointing at the full operator manual (the injected SKILL.md).
Alexgodoroja
left a comment
Alexgodoroja
left a comment
There was a problem hiding this comment.
Review — this PR is 6 distinct changes under a one-feature title
Read the full diff. The title/description cover only the next-steps self-heal (①), but the branch carries six independent changes. I've dropped inline markers ①–⑥ at each change boundary so the diff is navigable, with review notes attached.
| # | Change | Files |
|---|---|---|
| ① | Next-steps self-heal (titular) | appstore.go (call hook), appstore_nextsteps.go (+test) |
| ② | Same-version republish detection (.bundle-sha256, rebuilt) |
appstore.go (install hook), appstore_update.go (+test) |
| ③ | Updater auto-upgrades app adapters | cmd/updater/main.go (+test) |
| ④ | skills status/paths/info read-only (planTick) + accurate target list |
skills.go, zz_skills_test.go, go.mod/go.sum |
| ⑤ | install.sh: non-systemd / headless hosts |
install.sh |
| ⑥ | install.sh: full GET STARTED walkthrough |
install.sh |
Overall: quality is high across all six — pure-function splits, good test coverage, consistent resolveUnder confinement, fail-safe gates. Nothing looks broken.
Recommendations
- Scope — this is really 3–4 PRs. ①+② are related (both about propagating catalogue changes to the fleet); ③, ④, and ⑤/⑥ are independent. Splitting would make review, changelog, and bisect far cleaner. At minimum the body should enumerate all six: ③ ④ ⑤ ⑥ are currently invisible in the description.
- ③ is a supply-chain posture change — a signed catalogue republish now auto-deploys fleet-wide (that's how the aegis fix reaches hosts, by design). State it explicitly and document the
PILOT_UPDATER_NO_APP_UPGRADEopt-out. - ④ is an unmentioned real bugfix —
statuswas mutating disk just by being looked at. Arguably more user-facing than the titled feature; worth surfacing.
Not blocking — primarily a scope/documentation ask.
| fatalHint("invalid_argument", "json-args must be valid JSON", "%v", err) | ||
| } | ||
| } | ||
| // Lazily self-heal the cached graph before the call so it is current for |
There was a problem hiding this comment.
① Next-steps self-heal — the titular feature. The one-line hook: ensureNextStepsFresh runs before every appstore call. Steady state is a single stat; on a TTL miss it spends one bounded ~400ms fetch. Clean. One thing worth a sentence in the body: this is an inline, pre-IPC fetch on the call's critical path (once per app per 12h).
| // ensureNextStepsFresh is called once per `appstore call`, before the IPC call, | ||
| // so the cache is current for whichever outcome renders. It never blocks longer | ||
| // than nextStepsFetchBudget and never errors out of the call. | ||
| func ensureNextStepsFresh(appID string) { |
There was a problem hiding this comment.
① core. The marker design (.next-steps.checked / .next-steps.retry) and the authoritative-vs-transient split (fetchNextStepsAuthoritative returning ok) is the right shape — it correctly separates "no graph, stop asking for 12h" from "couldn't reach, retry in 2m". resolveUnder on every write/remove matches the existing cache path. LGTM.
| // timeout it reports ok=false (treated as a transient failure) and lets the | ||
| // goroutine finish and fall off on its own — it never writes anything, so an | ||
| // abandoned fetch can't race the cache. | ||
| func fetchNextStepsTimed(appID string, budget time.Duration) (*nextStepsGraph, bool) { |
There was a problem hiding this comment.
Minor (not blocking): fetchNextStepsTimed returns on the 400ms deadline, but the spawned goroutine keeps running fetchNextStepsAuthoritative until the underlying HTTP finishes. The buffered channel avoids a send-leak, but the fetch itself is only bounded by whatever timeout loadCatalogue/loadAppMetadata carry. Worth confirming that path has its own deadline — otherwise a slow host accumulates one lingering goroutine per call past TTL.
| // catalogue simply means no hints — never a failed install. | ||
| cacheNextSteps(finalDir, fetchNextStepsForInstall(m.ID)) | ||
|
|
||
| // Record the catalogue bundle sha we just installed so `outdated`/`upgrade` |
There was a problem hiding this comment.
② Same-version republish detection starts here (unrelated to ①). Records the catalogue bundle sha at install into .bundle-sha256 so outdated can flag a rebuilt-but-same-version adapter (the aegis argv-fix shape a version compare misses). Best-effort, catalogue-installs-only — correct.
| // that a version compare alone misses. Requires both shas; a pre-feature | ||
| // install with no recorded sha degrades to version-only detection. | ||
| // - "": up to date (or not enough information to say otherwise). | ||
| func outdatedReason(installedVer, catVer, installedBundleSHA, catBundleSHA string) string { |
There was a problem hiding this comment.
② core. outdatedReason is a clean pure function, nicely split out for testability and well covered (version / rebuilt / degrade-when-sha-missing). The graceful degradation — no recorded sha ⇒ version-only detection, exactly as before — is the right call for the pre-feature fleet.
| // does, so this adds automation, not trust. Opt out with | ||
| // PILOT_UPDATER_NO_APP_UPGRADE for hosts that want binary-only updates. | ||
| if !envBool("PILOT_UPDATER_NO_APP_UPGRADE") { | ||
| go appUpgradeLoop(*installDir, *statePath, *interval) |
There was a problem hiding this comment.
③ The updater now auto-upgrades app ADAPTERS, not just pilot binaries — a genuinely new behavior and the most consequential change in this PR, yet unmentioned in the body. It's gated correctly (enable-file + PILOT_UPDATER_NO_APP_UPGRADE opt-out) and re-runs the full signature/trust gate. But the net effect is a supply-chain posture change: any signed catalogue republish now auto-propagates to every auto-update host (by design — that's how ② reaches the fleet). Please state this explicitly and document the opt-out env var in the description.
| // skill state never mutates the filesystem — and never reports a file as | ||
| // "absent — next: create" in the same breath that a mutating tick just created | ||
| // it (the pre-write-state skew that ForceTick-backed status suffered from). | ||
| func planTick() (*skillinject.Report, error) { |
There was a problem hiding this comment.
④ skills status/paths/info are now read-only — planTick → skillinject.Plan instead of a mutating ForceTick. This is a real correctness fix: looking at skill state was writing to disk and could report "absent — next: create" about a file the same tick had just created. More user-impacting than the PR title implies, and the reason for the skillinject v0.2.4 bump in go.mod. Worth a line in the body. (The supported-list cleanup — drop Cursor, add PicoClaw/Goose — rides here too.)
| # --- Set up system service --- | ||
|
|
||
| if [ "$OS" = "linux" ] && command -v systemctl >/dev/null 2>&1; then | ||
| if [ "$OS" = "linux" ] && command -v systemctl >/dev/null 2>&1 && [ -d /run/systemd/system ]; then |
There was a problem hiding this comment.
⑤ install.sh: reach non-systemd / headless hosts — booted-systemd gate (/run/systemd/system), daemon-reload || true, guarded enable, and a fallback that prints the manual start path. Sound hardening for containers / WSL / CI. Independent concern.
| echo "" | ||
| echo "Get started:" | ||
| echo "============================================" | ||
| echo " GET STARTED" |
There was a problem hiding this comment.
⑥ install.sh: full GET STARTED walkthrough (heredoc). Docs/UX only, purely additive. Quoted heredoc (<<'PILOT_GET_STARTED') so no accidental expansion — good. Independent of everything above.
2 participants
Why
Next-steps graphs shipped (v1.12.7, catalogue #398), but the cache is written only at install and the call path reads only that local file. So the feature reached only apps installed after their graph shipped — the existing fleet, and any republished graph, got nothing without a manual
pilotctl appstore install <id> --force. That's a blocker: a feature that doesn't reach the installed fleet isn't really shipped.Root cause: the daemon never fetches the catalogue (it only scans local files), so pilotctl is the only actor that can refresh the cache, and it only did so at install.
What
ensureNextStepsFresh(appID)— called once perappstore call, before the IPC call, off the render path:app_versionbump to reach hosts).stat; network happens at most once per app per 12h. Two tiny marker files (.next-steps.checked,.next-steps.retry) gate it; an offline host backs off instead of re-fetching every call.resolveUnder(gosec clean).Proof
Live against the production catalogue on a fresh host: cleared the cache to simulate a pre-feature install → call #1 rendered the graph with no
--forceand populated the cache + marker → call #2 did zero network (strace: 0connect()), pure local read.Unit tests: marker freshness, steady-state short-circuit (no fetch when checked), failure→retry-cooldown, disabled no-op, traversal refusal. Full
cmd/pilotctlsuite green;gofmt/vet/gosecclean.Ships in the next release
This is client code — it reaches users when the next
v*tag is cut (like v1.12.7). No catalogue change needed; it reads the metadata already live on main.