fix: make inbox rollout source-owned and fail honest

[andrei-hasna]

Summary

• replace the missing legacy inbox executable dependency with a package-owned inbox skill contract
• add instructions managed-skills status/apply and attach reconciliation to profile apply/bootstrap
• verify @hasna/conversations version and the required watch flags before updating installed skill homes
• update installed skill contracts atomically, refuse symlink/non-regular targets, and never create a parallel inbox executable
• keep a bounded digest/DM/blockers fallback while hosted watch is degraded
• require hosted heartbeat plus observed channel and direct-message canaries before reporting the watcher ready
• correct the repo-only package secret-scan script so it no longer silently includes the user home

Hosted dependency

hasnaxyz/iapp-conversations#6 is merged, but hasna/conversations#95 remains open and no successful hosted watcher acceptance has been recorded yet. This PR therefore keeps the rollout explicitly degraded until:

1. conversations agents heartbeat --from <agent> --json succeeds;
2. conversations watch --from <agent> --all --interval 60000 --full-content surfaces one channel canary and one direct-message canary; and
3. the same acceptance pass records instructions managed-skills status --from <agent> --delivery-verified --json.

Heartbeat-only acceptance is not reported as ready.

Validation

• env -u HASNA_INSTRUCTIONS_API_URL -u HASNA_INSTRUCTIONS_API_KEY bun test — 628 pass, 0 fail
• affected re-validation — 14 pass, 0 fail
• bun run typecheck
• bun run build
• bun run kit:check
• bun run check:package-secrets
• bun run src/cli/index.tsx scan --all --limit 20 — no secrets
• bun run src/cli/index.tsx profile apply my-setup --dry-run — 1/64 changed; four inbox contracts predicted; manual fallback required
• bun run src/cli/index.tsx managed-skills apply --dry-run --json — changed 1, failed 0, four contracts, hosted heartbeat unverified, delivery unverified, fallback ready
• bun pm pack --dry-run --ignore-scripts — includes assets/skills/inbox/SKILL.md
• staged fail-closed hook and redacted gitleaks scan — clean

No station02 managed outputs were changed. No merge, publish, deploy, or reviewer dispatch was performed.

Fixes #63

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[andrei-hasna]

NO_GO

Exact candidate: 652a96f66f4af7233363ec514819bf6dddc6ff4e
Base: 36bcc103e7e7e18f59e2c51d3ff36b9833272883
Merge-result tree: 6180ce6c7c2cd77ddc16a6f4c2a1360ba8eb17ec (identical to the candidate tree; git merge-tree --write-tree exit 0)

Blocking finding

P1 — multi-home rollback can destroy a concurrent edit after a later target trips the stale-write guard.

src/lib/managed-skill-runtimes.ts:406-420 records each successfully written snapshot, but the catch path restores every earlier snapshot unconditionally with writeAtomic(snapshot.path, snapshot.content, ...). It does not first prove that the current file still contains the bytes written by this invocation.

I reproduced the reachable interleaving with two installed skill markers:

1. reconciliation atomically updated the first marker;
2. a concurrent writer changed that first marker to concurrent-first and changed the second marker to concurrent-second;
3. the second marker's pre-write check correctly threw managed skill changed after inspection; refusing a stale write;
4. rollback then overwrote the concurrent first-marker edit with its pre-inspection snapshot.

The probe's actual result carried "failed":1 and "first_after":"old-first\n"; the injected concurrent-first content was lost, while the second marker remained concurrent-second. This violates the stated atomicity/rollback gate and is material to managed session/data integrity under simultaneous managed-skills apply, profile/bootstrap reconciliation, or an operator edit.

Acceptance requires rollback to preserve an earlier target that changed after this invocation wrote it—for example, compare-and-restore only while the current bytes still equal this invocation's canonical write (or use an equivalent ownership/transaction mechanism)—plus a regression covering this interleaving and its direct failure path.

Evidence checked

• PR head/base re-read immediately before this comment; PR is mergeable/CLEAN and the merge-result tree equals the reviewed candidate tree.
• GitHub CI run 31196989825: Ubuntu and macOS package-manager secret guard, typecheck, test, and build steps all succeeded on this exact head.
• env -u HASNA_INSTRUCTIONS_API_URL -u HASNA_INSTRUCTIONS_API_KEY bun test: 629 pass, 0 fail.
• Affected lane: 10 pass, 0 fail; bun run typecheck: exit 0; bun run build: exit 0; bun run kit:check: ok storage-kit check.
• gitleaks git --redact --log-opts="36bcc103...652a96f": no leaks found.
• npm pack --dry-run --json: exit 0 and includes assets/skills/inbox/SKILL.md.
• Explicit profile apply my-setup --dry-run --from vitruvius: exit 0, 0/64 changed, and the runtime line was [dry-run] inbox via conversations watch — hosted heartbeat failed; manual fallback required.
• Installed Conversations is 0.5.29; current watch --help contains --from <agent>, --all, and --full-content; subscribe/subscriptions/digest/blockers help matches the documented fallback commands.
• Live heartbeat acceptance still fails, and the candidate reports healthy: false; the unrecorded hosted channel/DM canaries therefore remain honestly degraded and are not an additional blocker.

[andrei-hasna]

[REVIEW] NO_GO — #66 @ 652a96f — lens: correctness+security+gates, reviewer Xenon (1 of 1)

What I ran (un-piped, stdout and stderr captured separately):

• bun install — exit 0; setup only; 158 packages installed.
• bun run typecheck — exit 0; compiler gate passed (no test-count output).
• bun run test — exit 1; 624 pass, 5 fail, 629 total across 54 files. The failing tests were one session-CLI case plus four CLI output/ownership cases. Four print the local-store refusal caused by inherited HASNA_INSTRUCTIONS_API_URL / HASNA_INSTRUCTIONS_API_KEY; the fifth expected a spawned CLI add to exit 0 and received 1. The PR body’s env -u ... bun test result is not the declared bun run test gate requested for this review.

What I read:

• Exact head 652a96f66f4af7233363ec514819bf6dddc6ff4e against fetched origin/main 36bcc103e7e7e18f59e2c51d3ff36b9833272883.
• Full diff for all eight changed files, plus surrounding profile-apply, status, managed-skills, bootstrap, package/build, status-test, and session-render ownership/rollback paths.
• PR fix: make inbox rollout source-owned and fail honest #66 and issue BUG: instructions — inbox skill rollout omits the station02 executable #63 acceptance context.

Blocking P0/P1 findings:

1. P1, high confidence — successful reconciliation can irreversibly overwrite an unmanaged inbox skill. src/lib/managed-skill-runtimes.ts:18-23,170-185 classifies any regular file at one of five conventional skills/inbox/SKILL.md paths as managed based only on pathname presence. src/lib/managed-skill-runtimes.ts:379-413 then replaces every differing file with the packaged contract. This is reached automatically by ordinary profile apply (src/cli/index.tsx:1023) and bootstrap (src/cli/index.tsx:2218), with no ownership manifest/marker, no explicit adoption/force gate, and no durable pre-change snapshot. The new test at src/lib/managed-skill-runtimes.test.ts:147-183 demonstrates that arbitrary prior bytes are replaced. Impact: a user- or other-package-owned skill using the conventional inbox name loses its content after a successful command, and the in-memory rollback applies only when this invocation fails. This also contradicts the repository’s established session-render invariant, whose tests refuse even byte-identical unmanaged files. Remedy: prove package ownership before update (manifest/receipt or a bounded known legacy fingerprint), refuse unmanaged conflicts by default, require an explicit adoption/force path where appropriate, and persist rollback evidence for accepted managed updates. Add negative tests for unmanaged same-name files and restore behavior.
2. P1, high confidence — the repository’s declared test gate is red at this exact head: bun run test exits 1 with 624 pass and 5 fail. The gate must be hermetic under the supported ambient hosted-configuration environment or the declared script must establish its required test environment; a reviewer-only env -u wrapper is not the repository gate. Re-run the exact bun run test command after repair.

Non-blocking follow-ups:

• None.

Verdict: NO_GO. The ownership/adoption/rollback contract is real design work, so I am leaving the PR open rather than inventing a narrow patch.

[andrei-hasna]

Focused remediation candidate ready for the same reviewer.

Exact candidate: 9f3f0df73a225d9db9e7ea826fdaaa48d1dafc4d
Previous reviewed head: 652a96f66f4af7233363ec514819bf6dddc6ff4e
Base currently reported by the PR: 36bcc103e7e7e18f59e2c51d3ff36b9833272883

Scope is limited to the named rollback P1 and direct regressions:

• rollback restores an earlier file only while it is still a regular file whose current bytes equal the canonical content written by this reconciliation;
• a later concurrent edit is preserved and returned as a rollback conflict;
• an unchanged, still-owned earlier write restores its before-image;
• deterministic two-target regressions cover both interleavings.

Literal validation:

9 pass
0 fail
33 expect() calls

$ tsc --noEmit
exit 0

bun run build
exit 0

ok storage-kit check (expected v0.4.2) /home/hasna/.hasna/repos/worktrees/instructions/5093956b-68b1-4a27-962b-52367b36db29/src/generated/storage-kit

✓ Package-manager scan clean (2 file(s)).

[dry-run] inbox via conversations watch — hosted heartbeat passed; channel and DM delivery verification required
  skill contracts: 4

1/64 changed (my-setup on station02 linux/arm64)

npm pack --dry-run --json exited 0 and includes assets/skills/inbox/SKILL.md plus built dist.

Isolated exact staged-patch scan:

{
  "filesScanned": 1,
  "filesSkipped": 0,
  "errors": [],
  "findingCount": 0,
  "truncated": false
}

The exact commit-patch scan returned the same values. Commit has exactly one Agent: vitruvius trailer and no Co-Authored-By.

GitHub readback reports this exact head, PR state OPEN, mergeable MERGEABLE; Ubuntu and macOS CI are currently in progress. The hosted channel/DM canary remains honestly unasserted and is unchanged from the previously reviewed candidate.

Remaining gate: Mill's focused re-review of the named P1, its fix, and direct regressions only.

[andrei-hasna]

[REVIEW] NO_GO — #66 @ 9f3f0df — lens: correctness+security+gates, reviewer Timon (1 of 1)

What I read:

• git log --oneline origin/main..HEAD at current origin/main ca63d8a (exit 0).
• git diff origin/main...HEAD --stat and the complete diff for all eight changed files (exit 0), plus the surrounding CLI, status, runtime reconciliation, transaction, and test source.
• PR fix: make inbox rollout source-owned and fail honest #66 and issue BUG: instructions — inbox skill rollout omits the station02 executable #63 acceptance context.

What I ran:

• bun install — exit 0; setup only; 158 packages installed.
• bun run typecheck — exit 0; no pass/fail count is emitted by tsc --noEmit.
• bun run test — exit 1; 626 pass, 5 fail, 631 tests across 54 files.

Blocking P0/P1 findings:

1. P1 — the repository-declared test gate is not deterministic under the supported ambient API configuration. The exact required invocation, bun run test, inherits HASNA_INSTRUCTIONS_API_URL and HASNA_INSTRUCTIONS_API_KEY; local-store tests then fail closed in getDatabase, and a CLI subprocess test also exits 1. The literal gate output is 5 tests failed, 626 pass, 5 fail, and error: script "test" exited with code 1. This is a current required-gate failure, so the PR cannot merge at this head. Minimal remedy: make the declared test script isolate those two ambient variables before invoking Bun's suite, then rerun bun run test unchanged.

No other concrete, reachable, in-scope P0/P1 correctness or security defect survived review. The managed-skill path capability-checks the Conversations runtime, refuses symlink/non-regular targets, uses argv-safe process spawning, and preserves concurrent edits during rollback.

Non-blocking follow-ups:

• None.

[andrei-hasna]

Focused remediation pushed at 4d38c90ab07b5249d8eb92f078c24bbd6994abc5.

The repository-declared test script now removes the ambient HASNA_INSTRUCTIONS_API_URL and HASNA_INSTRUCTIONS_API_KEY only for the test process before invoking bun test. Production runtime behavior is unchanged. The affected gate was rerun through the required surface: bun run test — exit 0; 631 pass, 0 fail, 631 tests across 54 files.

Pre-commit staged scan: gitleaks protect --staged --redact --no-banner — exit 0, no leaks found. Repository package-manager scan: bun run check:package-secrets — exit 0, Package-manager scan clean (2 file(s)). Pre-push staged scan also exited 0 with no leaks found.

[andrei-hasna]

[REVIEW] GO — #66 @ 4d38c90 — lens: correctness+security+gates, reviewer Timon (1 of 1)

Focused verification of the sole blocking defect and its direct regression surface:

• The declared test boundary now isolates the two ambient Instructions API variables for the test process; production commands and runtime resolution are unchanged.
• bun run test — exit 0; 631 pass, 0 fail, 631 tests across 54 files.
• gitleaks protect --staged --redact --no-banner before commit and before push — exit 0; no leaks found.
• bun run check:package-secrets — exit 0; Package-manager scan clean (2 file(s)).
• Commit provenance readback ends with exactly Agent: Timon and contains no Co-Authored-By trailer.

Original-candidate gates retained from the initial pass:

• bun install — exit 0; setup only; 158 packages installed.
• bun run typecheck — exit 0; no pass/fail count is emitted by tsc --noEmit.

Blocking P0/P1 findings: none. The named required-gate defect is fixed and focused verification passes.

Non-blocking follow-ups: none.

[andrei-hasna]

[REVIEW] NO_GO — #66 @ 4d38c90 — lens: correctness+security+gates, reviewer Timon (1 of 1)

Correction: this supersedes my immediately previous GO at the same head. The mandatory pre-merge verdict readback prompted a recheck of an ownership boundary I had considered but incorrectly dismissed.

Blocking P0/P1 findings:

1. P1, high confidence — ordinary reconciliation can irreversibly overwrite a user- or other-package-owned inbox skill. src/lib/managed-skill-runtimes.ts:18-24 declares five conventional paths; inspectSkillMarkers at lines 188-205 treats any regular file at those paths as an installed managed contract based only on pathname presence. reconcileManagedSkillRuntimes at lines 469-504 classifies every differing file as stale and replaces it with the packaged bytes. This path runs automatically from ordinary profile apply and bootstrap. There is no package-ownership manifest/fingerprint, explicit adoption gate, or durable successful-write snapshot. The regression at src/lib/managed-skill-runtimes.test.ts:149-183 itself proves arbitrary prior bytes are replaced and the operation reports success. Impact: a same-name custom control-plane skill is silently lost on a currently reachable command path; the in-memory rollback only helps when the reconciliation fails, not after a successful overwrite.

Required remedy: establish package ownership before mutation (for example a package-owned receipt/manifest or a bounded known-legacy fingerprint), refuse unknown same-name files by default, provide an explicit adoption path if needed, and preserve recoverable rollback evidence for accepted updates. Add negative coverage proving an unmanaged same-name skill is not overwritten and positive coverage for a legitimately managed migration. This is real ownership/rollback design work, so I am leaving the PR open.

Gate state at this head:

• bun run typecheck — exit 0.
• bun run test — exit 0 after the focused test-boundary fix; 631 pass, 0 fail, 631 tests.
• staged and package-manager secret scans — exit 0, no findings.

Non-blocking follow-ups:

• None.

[andrei-hasna]

Closing unmerged after the fixed independent adversarial review returned NO_GO at exact head 4d38c90 against authoritative main e153c25. Managed-skill reconciliation validates only the leaf file, follows a symlinked parent directory, and can overwrite a regular source SKILL.md outside the managed home while reporting changed=1 and failed=0. Replacement trigger: reject every symlink ancestor before any managed-skill write; add a no-out-of-home-mutation regression; pass the affected tests and both required hosted builds against then-current main. A replacement that passes review still requires the package publish, station02 install/apply, asset-hash readback, heartbeat, and channel plus DM canaries.