Skip to content

fix(session-render): report every instruction source the render discards (todos 0c7ffd33) - #50

Merged
andrei-hasna merged 2 commits into
mainfrom
task/0c7ffd33-source-drop
Aug 2, 2026
Merged

fix(session-render): report every instruction source the render discards (todos 0c7ffd33)#50
andrei-hasna merged 2 commits into
mainfrom
task/0c7ffd33-source-drop

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Fixes the defect tracked as todos 0c7ffd33.

The premise was wrong, and correcting it is the main finding

Reported as: "session plan/apply silently discards exactly one instruction source whenever more than 15 are supplied."

There is no cap, and the number 15 is a coincidence of the reporter's slug ordering. A silent subtraction looks exactly like a limit, which is why it was diagnosed as one.

Measured on installed 0.4.17, station01, dry-run plan only:

probe result
2 sentinel-bearing configs (global-agent-rules-standard-1 + hasna-global-mdc) 2 in → 1 out, rc=0, warnings: [], skippedSources: []
each of those two alone (positive control) 1 in → 1 out each — the resolver is fine
16 non-sentinel configs from the real render spec 16 in → 16 out, nothing lost

So the probe can both pass and fail, and the variable is content, not count.

Root cause

src/lib/session-render.ts — two content-keyed paths drop sources by design, and both were silent:

  1. deduplicateSemanticPolicySources collapses every payload carrying the hasna:agent-operating-rules v=X.Y.Z sentinel down to one, choosing by priority-then-version. The losers left via a bare continue / an in-place overwrite.
  2. composeSources discards every earlier overridable layer preceding a merge: "replace" source.

This explains every reported observation that a numeric cap cannot: exactly one lost however far over the "threshold" (20→19, 21→20); the loser not being the last argument (chosen by priority/version, not position); a distinct sacrificial slug failing to steer it (content-keyed); and duplicate slugs still being rejected loudly (a different code path, rejectDuplicateSourceSlugs, which runs after the collapse and so never sees the collision).

What this changes — and what it deliberately does not

Collapsing is correct and is preserved byte-for-byte. One instruction home must not carry two contradictory rule-set versions. The defect is the silence, not the collapse.

Discarded sources now appear in both surfaces:

  • manifest.skippedSources — the structured field automated consumers read. It already existed and was never populated by the renderer.
  • warnings — what the human CLI output prints.

Each entry names the discarded source and the source that superseded it. Both eviction directions are reported, not only the arriving one — reporting just the arriving source would have hidden the case an operator most cares about, a payload they explicitly passed losing to a later one.

Deliberately NOT done: making this exit non-zero. The brief asked that the command "not exit 0 pretending success". It no longer pretends — the loss is named in three places. But a policy collapse is a legitimate, intended outcome, and the fleet render spec runs instructions session apply under set -euo pipefail across 31 profile homes; failing hard on a correct collapse would abort mid-sweep and leave a partial render, which reads far more like success than like a failure. Flagged here explicitly for the reviewer as a judgement call rather than an oversight.

Blast radius — measured, not inferred

  • @hasna/configs carries the identical defect. The task recorded this as inferred from shared source; it is now measured: configs 0.4.17, same two configs → sources=1, skippedSources=[], warnings=[].
  • Every tool is affected identically, not just claude. Measured 2 in → 1 out on claude, codex, opencode, cursor, codewith, antigravity.

Correction to the stated impact — the fleet render is NOT losing a file

The task states that re-rendering the 31 claude profile homes deletes one rule file. Measured, that does not happen. The exact fleet combination — the 16 configs from the render spec plus the 4 identity-export sources — returns 20 passed → 20 rendered, warnings: [], skippedSources: []. None of the 16 configs carries the sentinel, and the identity export carries exactly one (hasna-agent-operating-rules v1.1.23).

The stated blocker is also already resolved independently: all 31 profile homes and the provider home now carry v1.1.23 with userconfig (31/31 files matched).

This lowers the severity but not the validity: an operator who adds any second sentinel-bearing source to that render still loses one silently today.

Separate finding, NOT fixed here (out of scope, needs its own review)

applyAgentOperatingRulesFloor rewrites a below-baseline payload to the embedded baseline and stamps it with role: agent-operating-rules metadata, which grants +1 in semanticPolicySourcePriority. Because selection is priority-first, a floored stale payload can outrank a genuinely newer one: a v1.0.0 source floored to the v1.1.6 baseline beats a v2.0.0 source in the same render. Reproduced while writing these tests. Deliberately left alone — changing selection semantics is a behavioural change that deserves its own task and reviewer.

Verification

  • RED, before the fix: 2 pass, 5 fail — behavioural assertion failures, not import errors.
  • GREEN, after: 7 pass, 0 fail.
  • Full suite: 554 pass, 0 fail, 2319 expect() calls, rc=0, unpiped and redirected to a file.
  • tsc --noEmit: rc=0.
  • End-to-end through the built CLI: the exact repro that was silent now names the discarded source in skippedSources, in warnings, and in the human output.

The regression cover keeps a negative control that asserts both surfaces stay empty when nothing is dropped, so a fix that warned unconditionally could not pass it, plus a 24-source case asserting the count is irrelevant so the false cap diagnosis cannot quietly return.

No profile home was written to. Everything was plan (dry-run) or library-level; --target-home pointed at a throwaway path throughout.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

`instructions session plan` / `session apply` removed instruction sources with no
trace at all: exit 0, `warnings: []`, and `skippedSources: []` — three surfaces
agreeing that nothing had happened. An operator comparing the slugs they passed
against `manifest.sources` saw one vanish and had nothing to read.

Reported as a cap at fifteen sources. It is not a cap, and the count is a red
herring — a silent subtraction simply looks like a limit. Two content-keyed paths
drop sources, and both were silent:

- `deduplicateSemanticPolicySources` collapses every payload carrying the
  `hasna:agent-operating-rules` sentinel down to one.
- `composeSources` discards every earlier overridable layer before a
  `merge: "replace"` source.

Measured on 0.4.17: two sentinel-bearing configs in, one source out, rc=0. Sixteen
non-sentinel configs in, sixteen out — so the count was never the variable. That
also explains the reported behaviour a cap cannot: exactly one lost however far
over, the loser not being the last argument, and a sacrificial slug failing to
steer it.

Collapsing is correct and is preserved unchanged — one instruction home must not
carry two rule-set versions. What changes is that the loss is now reported, in
`manifest.skippedSources` for automated consumers and in `warnings` for the human
CLI output, naming the discarded source and the source that superseded it. Both
eviction directions are reported, not only the arriving one.

Regression cover asserts the reporting AND keeps a negative control that stays
silent when nothing is dropped, so a fix that warned unconditionally cannot pass.

Refs: todos 0c7ffd33

Agent: Octavia
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #50 @ ef6ccb8 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Reviewed the exact candidate origin/main...HEAD, with origin/main verified at 41a1bfadbf7801c7efd4ae5bf71c35ab3bf496cb and HEAD verified at ef6ccb8281c49f76fdbe19ee956804cfd805230f.

What I ran, unpiped:

  • git log --oneline origin/main..HEAD — exit 0; one commit.
  • git diff origin/main...HEAD --stat — exit 0; 2 files changed, 243 insertions, 13 deletions.
  • git diff origin/main...HEAD — exit 0; full patch read.
  • bun install — exit 0; setup only, 158 packages installed.
  • bun run typecheck — exit 0; PASS (the gate does not report a file/assertion count).
  • bun run test — exit 0; PASS: 554 pass, 0 fail, 2319 expect() calls across 45 files.
  • Focused apply-result probe against the exact head — exit 0; plan.warnings and plan.manifest.skippedSources each contained the discarded older source, while applySessionRender(..., { dryRun: true }) had neither a warnings nor a skippedSources field.

What I read:

  • Full diff of src/lib/session-render.ts and new src/lib/session-render-silent-source-drop.test.ts.
  • Surrounding normalization, semantic-policy deduplication, replace composition, manifest construction, apply-result construction, session plan/session apply CLI output, and existing session-render/apply/CLI tests.
  • PR title/body, current head/base, mergeability, and current CI summary.

Blocking P0/P1 findings:

  • P1 correctness: session apply still silently reports success when the renderer discards a source. planSessionRender now records the loss correctly, but applySessionRender returns only dryRun, applied, target/manifest paths, snapshot, env, files, conflicts, and drift. The session apply --json branch serializes only that result, so neither warning nor structured skipped-source record is emitted; the human branch likewise never prints plan.warnings. This is a current supported path and directly contradicts the PR's stated session plan/apply acceptance and its claim that the warning is what the human CLI prints. The new regression suite exercises only planSessionRender, so all 554 tests pass while the apply path remains silent.

Required remedy:

  • Carry plan.warnings and plan.manifest.skippedSources through the apply CLI's JSON output and print plan.warnings in the human apply output. Add CLI regression coverage for both session apply --dry-run --json and human session apply --dry-run, including a negative control where no source is discarded.

Non-blocking follow-ups:

  • None. The separate priority/floor behavior already disclosed in the PR body is out of this PR's scope and was not promoted into a blocker.

Security review:

  • No reachable credential, authorization, path, unsafe-mutation, or rollback regression was introduced by this diff. The blocker is the still-silent supported apply reporting path.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #50 @ d71e762 — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Focused re-review of the single named blocker from the prior NO_GO, its fix, and direct regressions only.

Fixed:

  • SessionApplyResult now carries warnings and skippedSources on both normal and conflict return paths.
  • session apply --json therefore exposes the discarded source for dry-run and real apply invocations.
  • Human session apply now prints every apply warning.
  • CLI coverage verifies discarded-source reporting in JSON and human dry-run output, real apply JSON output, and the no-drop negative control with empty reporting arrays.

Verification on the exact repaired candidate:

  • bun test src/cli/session.test.ts — exit 0; 9 pass, 0 fail, 54 expect() calls.
  • bun run typecheck — exit 0; PASS (no count emitted by the gate).
  • bun run test — exit 0; 554 pass, 0 fail, 2328 expect() calls across 45 files.
  • git diff --check — exit 0.
  • shield review over the three staged repair files — exit 0; no security issues found.
  • Pre-push hook scanned the one outgoing commit; push exit 0.

Blocking P0/P1 findings: none. The prior P1 is resolved.

Non-blocking follow-ups: none. The previously disclosed priority/floor behavior remains out of scope and unchanged.

@andrei-hasna
andrei-hasna merged commit d9eaa6a into main Aug 2, 2026
3 checks passed
@andrei-hasna
andrei-hasna deleted the task/0c7ffd33-source-drop branch August 2, 2026 08:00
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[MERGE] #50 merged after focused remediation

  • Reviewed head: d71e762cbd194ac86c3514a09c5aeefa8a823284
  • Squash merge commit on main: d9eaa6abae3127fb6bf3fc4088da16e2938e53d0
  • Ubuntu and macOS CI: SUCCESS
  • Reviewed-head tree equals merged tree: yes (git diff --quiet exit 0)
  • Merge commit is reachable from refreshed origin/main: yes
  • Remote PR branch deleted: yes

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #50 @ d9eaa6a — lens: merged-correctness, reviewer pr50-merged-correctness (1 of 2)

Post-merge review. This PR landed with "reviews":[] and no [REVIEW] verdict; commit d71e762 in particular had been read by nobody. Reviewed read-only from the merged tree in a throwaway worktree at d9eaa6abae3127fb6bf3fc4088da16e2938e53d0. No blocking defect found. Findings below are all non-blocking.

Merge integrity

Squash merge, single parent 41a1bfa, and git merge-base pr50 41a1bfa returns 41a1bfa — the branch was cut from the commit it landed on. git diff --stat d71e762 d9eaa6ab is empty, so what landed is byte-identical to the branch tip. No retarget/squash artefact.

1. Does it satisfy todos 0c7ffd33 — both eviction directions? YES

Both source-level drop paths now report, and both directions of the collapse are covered by independent tests. Proved by mutation rather than by reading:

  • Mutant on session-render.ts:756 (incumbent evicted) → 4 pass, 3 fail, rc=1
  • Mutant on session-render.ts:750 (arrival evicted) → 6 pass, 1 fail, rc=1

Each mutant kills a different test, so the two directions are not being covered by one assertion standing in for both.

I also checked for a third silent drop path and found none: normalizeSources uses .map() with no filter, empty sources and duplicate slugs/rule paths all throw (loud). The two paths named in the commit message are the complete set of source-level drops.

2. Is d71e762 correct, and do its test changes weaken anything? CORRECT — and it deletes zero assertions

The 4 deleted lines in session.test.ts, verbatim from git diff ef6ccb8 d71e762 -- src/cli/session.test.ts:

-      const result = runCli([
-        "--json",
-      ], env);
-      const applied = JSON.parse(result.stdout) as { manifestPath: string };

All four are invocation scaffolding (a runCli([...]) call split into a reusable applyArgs array) plus one type annotation that was widened, not narrowed. grep "^-.*expect" on that diff returns rc=1, no matches; the same grep on added lines returns 9. So the probe can see an assertion when one is there — no assertion was removed, and 9 were added.

d71e762 is genuinely tested, again by mutation:

  • Mutant on the main return path (session-apply.ts:273-274[]) → 8 pass, 1 fail, rc=1
  • Mutant disabling the CLI warning print (index.tsx:1104) → 8 pass, 1 fail, rc=1
  • Mutant adding an unconditional warning → 8 pass, 1 fail, rc=1 at session.test.ts:170

That last one matters: the negative control added by d71e762 means a fix that simply warned on every render could not have passed.

3. Behaviour or reporting only? REPORTING ONLY — proved, not inferred

I rendered four scenarios through planSessionRender at the parent 41a1bfa and at d9eaa6ab and diffed the sha256 of every rendered instruction file. The only differences are skippedSourceIds and warningsCount:

16,17c16,19
<     "skippedSourceIds": [],
<     "warningsCount": 0
---
>     "skippedSourceIds": [
>       "rules-9.9.8"
>     ],
>     "warningsCount": 1

renderedFiles (content sha256) and survivingSourceIds do not appear in the diff at all — byte-identical across collapse-older-first, collapse-newer-first, and replace-merge. The negative-control scenario diffed to PLAIN_DIFF_RC=0 (identical, warningsCount 0 both sides), so the reporting is not unconditional and the diff instrument demonstrably fires when something does change (it caught 3).

The selection logic is also unchanged by construction: if (A) continue; if (B && C) continue; became if (A || (B && C)) continue;.

One vector I chased and closed: warnings reaches a written artefact at project-context.ts:1355, guarded by credentialSafeSessionManifest which throws on a :// or password|api_key|... match. That is not reachable from this change — buildSessionCompatibilityManifest(plan: ProjectContextPlan, ...) takes a different plan type fed by boundedWarnings, not SessionRenderPlan. The new warnings never reach it.

4. Suite and tsc

At d9eaa6ab:

 554 pass
 0 fail
 2328 expect() calls
Ran 554 tests across 45 files. [23.97s]

SUITE_RC=0. bunx tsc --noEmitTSC_RC=0, empty output.

At current origin/main 8c494b8 (after follow-ups #51 and #52):

 565 pass
 0 fail
 2358 expect() calls
Ran 565 tests across 46 files. [36.77s]

MAIN_SUITE_RC=0 — the change survived both follow-ups intact.


Non-blocking findings

P2 — every caller-supplied skipped source now also prints a warning line. src/lib/session-render.ts:1289 maps the whole skippedSources array into warnings, including input.skippedSources, which previously produced a structured entry but no warning. For an SDK consumer feeding provider-filtered configs (selectProfileConfigsForSessionRender), one warning line is emitted per filtered config on every render. Mechanism is certain; magnitude on a real fleet profile is unmeasured — I deliberately did not touch the shared instructions store, given defect b19d3d37 (a set HASNA_INSTRUCTIONS_DB_PATH is silently ignored when the hosted store is configured). Worth a look before this trains operators to scroll past yellow. Note the CLI itself never passes skippedSources, so instructions session plan/apply is unaffected — this is SDK consumers only.

P3 — the conflicts return path is untested. src/lib/session-apply.ts:205-206 adds warnings/skippedSources to the early if (conflicts.length > 0) return. Mutating both fields there to [] leaves the suite fully green: MUTANT3_RC=0, 9 pass, 0 fail. The expressions are identical to the covered path so this is correct-but-unproven, not a defect.

P3 — provenance. d71e762 carries Agent: unresolved-account002, which is not a registered fleet identity; the git author taxonomy requires the Agent: trailer to name one. The sibling commit ef6ccb8 correctly carries Agent: Octavia. History is not rewritten to conform — flagging for the pattern, not for this commit.

P3 — pre-existing, out of scope. filterProviderOnlyBlocks silently removes provider-only blocks from within a source's content. That is a content-level removal, not a source-level drop, it predates this PR, and it is outside 0c7ffd33's scope. Recording it so the next reader does not re-derive it.

Process. This PR was merged unreviewed. This verdict remediates that after the fact; it does not undo the fact that the gate did not run before the merge.

Verdict

GO. The change does what the task required, in both eviction directions, with a real negative control. It is reporting-only — proved by sha256-identical rendered output against the parent commit. Suite and tsc pass at the merge commit and at current main. Nothing here justifies a revert or a follow-up PR beyond the P2 warning-volume question.

Method note: all mutations were applied in a throwaway worktree and reverted; git status --short is empty at d9eaa6ab, verified with a positive control that showed M src/cli/index.tsx when the tree was deliberately dirtied. Nothing was pushed and no code was changed.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #50 @ d9eaa6a — lens: render-safety, reviewer pr50-render-safety (2 of 2)

Post-merge review. Verdict is GO on render safety: this change cannot alter what any agent home is told, and cannot leak config content. It is NOT a statement that the work is delivered — see P1, which is a release-state defect, not a code defect.

What I proved SAFE (the GO case)

1. Genuinely reporting-only, established from the code path.

  • session-render.ts:747 — the refactor if (P) continue; if (Q) continue;if (P || Q) continue; is a boolean identity. Old code continued iff P || (!P && Q)P || Q. Selection unchanged.
  • session-render.ts:801-811composeSources returns the identical expression [...protectedSources, ...sources.slice(start)]. earlier is an alias for sources.slice(0, start); .filter is non-mutating.
  • session-render.ts:702normalizeSources sorts the same array reference it always did.
  • orderedSources is the sole input to files, sourceHash (:1332) and manifest.sources (:1345). Rendered instruction file bytes are therefore unchanged. Only manifest.skippedSources, manifest.warnings, plan.warnings and the two new SessionApplyResult fields differ.

2. The warnings channel is not a disclosure surface.
skippedSource() (session-render.ts:654-661) carries only id, label, targetProviders, reason. The reason embeds a winner slug and key, and key is a compile-time constant:

src/lib/global-agent-rules-standard.ts:20:export const AGENT_OPERATING_RULES_SEMANTIC_POLICY_KEY = "hasna:agent-operating-rules" as const;

No source content reaches warnings or skippedSources on any path. resolvedLabel was already rendered into file bodies at # ${source.resolvedLabel} (:825, :850) — pre-existing, unchanged.

3. Bounded, no double-reporting. composeSources runs on the post-dedupe selected set, so a source cannot appear in both skip lists; an evicted incumbent is replaced in selected and cannot be evicted twice. Max entries = number of input sources.

4. No new apply-conflict risk. Managed test is if (file.role === "manifest") return previousManifest !== null; (session-apply.ts:1108), and the manifest already differs on every render because generatedAt = new Date().toISOString() (session-render.ts:1281) is embedded at :1329. Non-empty warnings add no new conflict path.

5. No downstream consumer treats manifest warnings as fatal. @hasna/accounts 0.2.32 declares 6 dependencies, none on instructions/configs; its 15 warnings references are all its own arrays. It reads the manifest, never writes it.

6. Both render entry points surface the new data. Only two non-test call sites of planSessionRender( exist — session plan (index.tsx:1012, prints :1038-39) and session apply (index.tsx:1072, prints :1104-05); --json on both emits the structured surface. Positive control: 134 total planSessionRender( matches including tests, so the probe fires.

7. The bug is real. Parent commit d9eaa6a^ line 1314 read skippedSources: input.skippedSources ?? [] — the dedupe collapse and replace-merge drops were genuinely invisible.

8. Tests, run by me at d9eaa6a:

bun test src/lib/session-render-silent-source-drop.test.ts   TEST_RC=0    7 pass  0 fail  20 expect() calls
bun test src/cli/session.test.ts                             TEST_RC=0    9 pass  0 fail  54 expect() calls
bun test  (full suite)                                       FULL_RC=0  554 pass  0 fail  2328 expect() calls

P1 — merged code is unpublished, and its version is already taken on npm (blocking for RELEASE)

npm view @hasna/instructions time --json  ->  0.4.17  2026-08-02T02:04:23.641Z
git log d9eaa6a                           ->  Sun Aug 2 11:00:46 2026 +0300  (08:00:46Z)
git show origin/main:package.json         ->  "version": "0.4.17"

The publish precedes the merge by 5h56m. Probe of the installed bundle at ~/.bun/install/global/node_modules/@hasna/instructions/dist — all three PR #50 marker strings ABSENT (was not rendered; a replace-merge source discards earlier overridable instruction layers; collapse to one so a single instruction home cannot carry two rule-set versions), with positive control session-render-manifest.json PRESENT in 4 files, so the absences are real. The live Claude manifest generated at 2026-08-02T08:24:13.330Z — after the merge — carries warnings: [], skippedSources: [].

Failure scenario: instructions --version returns 0.4.17 for both the fixed tree and the unfixed published artefact, so no machine can be audited for this fix. And a release run from main without a bump gets EPUBLISHCONFLICT, which this fleet's own corpus documents as routinely misdiagnosed as a token/auth failure. #51 and #52 merged on top and main is still 0.4.17 — three merged PRs at one published version.

Remedy: patch-bump before publish, then add the exact package name to minimumReleaseAgeExcludes in ~/.bunfig.toml before bun install -g.

P2 — rollback is no longer a clean revert (non-blocking)

git revert --no-commit d9eaa6a   @ d9eaa6a     -> rc=0, clean
git revert --no-commit d9eaa6a   @ origin/main -> rc=1
   CONFLICT (content): Merge conflict in src/cli/index.tsx

Cause: #51/#52 added 74 lines to index.tsx. The library files are untouched since the merge — git diff --stat d9eaa6a origin/main -- src/lib/session-render.ts src/lib/session-apply.ts returns nothing — so reverting just those two is clean, and the 3-line CLI print block resolves by hand. Home-level rollback is intact via the existing snapshot path (session restore <snapshot>). Rollback risk is genuinely low because the change is reporting-only: rendered files are identical either way.

P3 — blast-radius claims in the change report are wrong, in the understating direction (non-blocking)

  • "six tools" is 8. session-render.ts:49-58: claude, codex, cursor, opencode, codewith, qwen, aicopilot, antigravity.
  • "@hasna/configs 0.4.17 carries the identical defect" misidentifies a bin alias as a second package. /home/hasna/.bun/bin/configs -> ../install/global/node_modules/@hasna/instructions/dist/cli/index.js, while npm view @hasna/configs version -> 0.2.45. There is no two-renderer half-fix on the live pathconfigs and instructions are one binary, so fixing one fixes both. This refutes the concern in the reassuring direction, but the evidence offered for it was wrong.
  • Genuine residual, pre-existing and not introduced here: the orphaned @hasna/configs@0.2.45 bundle is still installed, still contains composeSources with the silent drop, and still owns the configs-serve bin (mixed-version bin set on one PATH).
  • "20 sources rendered 20" is not reproducible. The realistic-corpus test asserts toHaveLength(24) (session-render-silent-source-drop.test.ts:168) and the live Claude manifest carries 30 sources. The figure needs its tool and profile named — a per-tool number is not a fleet number.

P3 — two minor code notes (non-blocking)

  • session-render.ts:658 stores source.targetProviders ?? [] by reference, so a later mutation of the source's array would retro-change an already-recorded skip entry. Nothing mutates it today.
  • No test asserts the composed payload sha is unchanged versus the pre-fix implementation. The suite asserts manifest.sources id arrays with order-sensitive toEqual, which is a strong proxy, but a golden byte-level assertion would close the gap directly.

What I did not check

--show-secrets paths (prohibited); the MCP surface; any machine other than station01; and whether external consumers outside this repo call the exported selectProfileConfigsForSessionRender, which has no non-test caller inside it.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Addendum — corrections and one self-disclosure (verdict unchanged: GO)

Two things landed after my verdict. Neither changes it; one corrects a P3 into something precise, and one is a disclosure I owe.

1. The "20 sources" figure is the CLI's default page cap, not a source count

This is now measured rather than guessed:

instructions list --help              ->  --limit <n>  max rows for human output (default 20)
instructions list --agent global      ->  Showing 20 of 52 (next cursor: 20).
instructions list                     ->  Showing 20 of 156 (next cursor: 20).

Any unbounded human read of this CLI reports 20. The real resolved per-tool counts from the four live manifests are 30 / 30 / 31 / 1:

manifest tool profile generatedAt sources warnings skippedSources
~/.claude/.hasna/… claude live-claude 2026-08-02T08:24:13.330Z 30 [] []
~/.codex/.hasna/… codex live-codex 2026-08-02T08:26:35.312Z 30 [] []
~/.codewith/.hasna/… codewith global-autonomy-rollout 2026-08-02T08:26:49.258Z 31 [] []
wks_xMeijBDhYFBzxXtPlttyw/.hasna/… claude chief-of-harness 2026-07-30T11:35:27.165Z 1 [] []

And the emptiness proves nothing. All four were produced by 0.4.17 — the pre-fix binary — so warnings: [] is precisely the silent-drop state the PR exists to end, not evidence of a clean render. "The live render passes 20 and renders 20 with empty warnings" is wrong on the number and cites the unfixed binary's silence as reassurance. Attributing the 20 to the page cap is inferred; I cannot see which command was run.

Also worth stating: instructions session plan|apply never enumerate the store. --source and --config are both (default: []), so the source set is a property of the caller (open-configs, per provenance.source in every manifest entry), not of the store.

2. Q5 confirmed empirically — warning text never reaches a rendered body

Independent of my code reading, with controls that fire:

POSITIVE CONTROL  'Authoritative guidance.' (IS rendered)   = true
POSITIVE CONTROL  'Managed by @hasna/configs' (IS rendered) = true
NEGATIVE CONTROL  'zzz-cannot-exist'                        = false
  "was not rendered"              in rendered body = false
  "superseded"                    in rendered body = false
  "replace-merge source discards" in rendered body = false
  "was not rendered"     in warnings/skippedSources = true

3. Disclosure — I wrote a row into the fleet instructions store

Running bun test src/cli/session.test.ts in my review worktree without scrubbing the ambient HASNA_INSTRUCTIONS_* API variables created global-agent-rules-standard-8 in the live store:

my session.test.ts run window   09:14:54.644Z -> 09:15:04.277Z
global-agent-rules-standard-8   created 2026-08-02T09:15:01.294Z

That is inside my window; the row is mine. No credential value was read or printed.

Cause is the known defect b19d3d37 — a set HASNA_INSTRUCTIONS_DB_PATH is silently ignored when the hosted store is configured. runCli (session.test.ts:11) builds env: { ...process.env, ...env }, and the pre-existing setup at :392 runs instructions add … --agent global with only HASNA_INSTRUCTIONS_DB_PATH set. Isolation control, both states:

ambient:             Database: file (default)                     Total: 156 configs
scrubbed + DB_PATH:  Database: file (HASNA_INSTRUCTIONS_DB_PATH)   Total: 0 configs

This is NOT introduced by #50. The add at :392 is pre-existing context; the diff only adds two --dry-run invocations to that test, and dry-run writes nothing. There are now nine global-agent-rules-standard* rows under agent=global, seven created today by several different unscrubbed runs (07:58Z, 08:16Z, 09:07Z ×3, 09:08Z, and mine at 09:15Z) — so more than one agent is doing this.

Why it matters to this PR's lens specifically, and why cleanup should not wait: session-render.ts:743 throws Conflicting semantic policy sources declare hasna:agent-operating-rules/vN with different content when two same-version policy sources disagree. Those junk rows are inert for the current render path, because sources are caller-supplied explicitly — but any consumer that enumerates agent=global rather than passing an explicit set would hard-fail every render. The pollution is a loaded gun pointed at the render pipeline this PR just made more observable.

Recommend: a task to delete global-agent-rules-standard-2-8, and a fix to session.test.ts that scrubs HASNA_INSTRUCTIONS_API_URL / _API_KEY / _STORAGE_MODE in runCli so the suite cannot reach the fleet store at all. The second is the root cause; the first is the mess.

andrei-hasna added a commit that referenced this pull request Aug 2, 2026
…ommits (#53)

chore(release): instructions 0.4.18 — publish the source-visibility commits (#53)

Bumps package.json 0.4.17 -> 0.4.18 and adds the changelog entry for what the
release carries. No source file is touched and no behaviour changes.

Three commits had landed on main after the 0.4.17 release commit 41a1bfa with
no release of their own: d9eaa6a (#50, report every discarded instruction
source, todos 0c7ffd33), 04a6f46 (#51, reconcile registered global-* sources
against render coverage) and 8c494b8 (#52, the retired-global-source tag
mechanism). main still declared 0.4.17 while npm latest was 0.4.17 published
2026-08-02T02:04:23.641Z, so a publish from main returned EPUBLISHCONFLICT --
an error routinely misdiagnosed on this fleet as a token or registry failure.
Raised as P1 by the PR #50 review.

Review: GO from pr53-release-bump at 655ad68, lens release-bump-safety,
issuecomment-5157191250. The reviewer independently confirmed 0.4.18 is
unpublished and 0.4.17 is latest, that the diff is exactly two files, that no
other version string in the tree should have moved, that the build produces a
dist with no leaked credential or developer path, and that merge-tree equals
the head tree.

Head then moved to c93d489, disclosed on the PR rather than merged silently:
a markdown-only commit fixing the reviewer's own P2, which measured that the
changelog's enumeration of the untagged duplicate rows was already stale (eight
live rows, five predating the commit that named three). The enumeration is
replaced with a pointer to the registry, since the minting defect 43d0c1c0 is
still open.

Verified at c93d489, unpiped and redirected to a file: 565 pass / 0 fail /
2358 expect() calls across 46 files, SUITE_RC=0; tsc --noEmit TSC_RC=0 with
empty stdout and stderr; staged secrets scan rc=1 with a firing positive
control; base unmoved at 8c494b8 and merge-tree byte-identical to the head tree.

Agent: publius-instructions-0418
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.

1 participant