fix(heartbeat): stop reporting an already-imported skill as absent (BLO-31993) - #1679
fix(heartbeat): stop reporting an already-imported skill as absent (BLO-31993)#1679allyblockcast[bot] wants to merge 6 commits into
Conversation
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
…LO-31993) BLO-7991 AC2's notice told two agents their skills were "not in the company skill library" and to import them, while both skills had been in the library since 2026-07-21. The counts and key names were right; the reason and the remediation — the actionable half — were wrong. `computeUnmaterializedDesiredSkills` had only one branch for "no runtime entry", and called it `absent`. But `listRuntimeSkillEntries` drops a key for two structurally different causes: no `companySkills` row at all (never enters the filter loop), or a row whose `resolveRuntimeSkillSource` returned null and was dropped by a bare `continue`. The second is what a rolling materialization sweep looks like from here — `materializeRuntimeSkillFiles` is rm -rf -> mkdir -> per-file write, not atomic, so a run launching mid-sweep sees the row without the files. In the live repro the two keys were rewritten at 11:03:54Z and 11:04:22Z, i.e. after the 10:53:52Z prompt was built. Those two causes need opposite advice. Re-importing does not fix a skill that is already imported, and the notice additionally asserted "retrying will not fix it" over a condition that self-heals. Split the branch on the catalog, which is the source of truth for "is this in the library": catalog row exists -> materialization_pending (already imported, transient) no catalog row -> absent (import guidance, unchanged) `listCatalogSkillKeys` is a bare key projection over `companySkills` — no `resolveRuntimeSkillSource`, no `ensureSkillInventoryCurrent`, no filesystem access — so the AC2 hot path stays `reconcileInventory: false` as its AC requires. It is called only when a first, catalog-free pass produced at least one `absent`, so the happy path and the `unresolved_source`-only path pay nothing, and it is wrapped: refining a warning's wording must never be able to fail run setup, so a lookup error logs and falls back to the prior behavior. Classification can only relabel a reason, never add, drop or reorder a reported key, so `N configured, M available` and the named set are unchanged — asserted directly. With nothing pending, the closing paragraph is reproduced byte for byte, so BLO-7991 AC2's own wording is untouched. Why CI missed it: `heartbeat-unmaterialized-desired-skills.test.ts` built `runtimeSkillEntries` as the sole source of truth, so "key absent from the array" trivially meant `absent` and the production state "catalog row exists, inventory row does not yet" was unrepresentable. The unit test now takes the catalog as a separate axis; `heartbeat-runtime-skills.test.ts` adds a real run against a real `companySkills` row, which is the half that proves the call site actually consults the catalog rather than the pure function merely being correct. Both fail against the pre-fix tree (6 and 1 respectively) and the 20 pre-existing cases stay green.
91a2822 to
60981c9
Compare
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 60981c9
Critical Issues (0)
Important Issues (2)
-
[code / error-handling]
server/src/services/heartbeat.ts:2811—materialization_pendingpromises self-healing for a class that also contains permanent, non-self-healing failures.Tracing the seam: on the AC2 path
listRuntimeSkillEntriesis called withoutmaterializeMissing: false, soresolveRuntimeSkillSource(company-skills.ts:5396) returnsnull— and the key is dropped with the barecontinue— only whenmaterializeRuntimeSkillFilesormaterializeVersionSnapshotthrows.materializeRuntimeSkillFilesthrows deterministically atcompany-skills.ts:5264("Company skill could not be materialized because its stored SKILL.md copy is missing.") whenever noSKILL.mdcontent can be produced. That is a hard fault: it will throw identically on every subsequent run.So "catalog row present, no runtime entry" is not a transient-only state, but the notice asserts it is, flatly: "a materialization sweep has not finished writing their files to the runtime volume; that clears on its own, so a later run will pick them up." For a skill with a broken/incomplete
fileInventorythat is false, and the change also removes the one signal that previously discouraged waiting (retrying will not fix itno longer covers these keys). That is the same failure shape as the bug being fixed, with the sign flipped: previously an already-imported skill was told to re-import; now a permanently-unmaterializable one is told to wait forever.The PR's own integration test is the evidence, and it is candid about it —
heartbeat-runtime-skills.test.ts:434induces the state withfileInventory: [{ path: "reference.md" }](noSKILL.md), i.e. exactly the permanent5264throw, and then asserts the transient wording. The comment at:395acknowledges "different trigger, byte-identical state at the classification seam" — which is precisely why the wording cannot safely assert the trigger.- Recommended: either (a) soften the remediation to state what is known rather than what is inferred — the library row exists, the runtime files are not published, do not re-import, and a later run may pick them up — while keeping "report it if it persists"; or (b) distinguish the two throw paths (a thrown
unprocessablefrom5264is a hard fault; an FS/race error is not) and keep the self-healing promise only for the latter. Renaming the reason to something trigger-neutral (runtime_files_unpublished) would also stop the enum itself asserting transience.
- Recommended: either (a) soften the remediation to state what is known rather than what is inferred — the library row exists, the runtime files are not published, do not re-import, and a later run may pick them up — while keeping "report it if it persists"; or (b) distinguish the two throw paths (a thrown
-
[code]
server/src/__tests__/heartbeat-unmaterialized-desired-skills.test.ts:173— unintended line-join corrupting a pre-existing test.it("normalizes whitespace and de-duplicates declared keys", () => { expect(computeUnmaterializedDesiredSkills({
The diff (
@@ -75,8 +170,7 @@) removes the newline + indentation after the arrow, collapsing two lines into one 116-character line. It is unrelated to BLO-31993, is not mentioned in the PR description, and the repo has no lint/format script inserver/package.jsonor the rootpackage.json, so CI will not catch it. The test still passes; this is a hygiene revert, not a behavior fix.- Recommended: restore the original two-line form.
Suggestions (2)
- [code]
server/src/services/heartbeat.ts:2796-2797—hasPending/hasConfigFaultare derived from the fullmissingarray, but the bullets rendered are onlyshown(:2783, capped atUNMATERIALIZED_SKILL_NOTICE_MAX_KEYS = 20). With more than 20 unmaterialized keys, the notice can print a remediation paragraph about "the keys whose runtime files are not published yet" when no such key appears in the visible list — the reader sees advice with no referent. Deriving the two flags fromshown(or naming the counts) keeps the prose and the list consistent. - [efficiency]
server/src/services/company-skills.ts:5438—listCatalogSkillKeysapplies.orderBy(asc(companySkills.key)), but the sole consumer feeds the result straight into aSet(heartbeat.ts:2695). The ordering is discarded; dropping it removes a sort from a path that runs during run setup.
Strengths
- The fix is aimed at the actual discriminator.
listRuntimeSkillEntriesgenuinely cannot answer "is this in the library", and consultingcompanySkillsdirectly is the right source of truth rather than inferring it from the entries array. listCatalogSkillKeysis scoped tightly and the docstring says why: noresolveRuntimeSkillSource, noensureSkillInventoryCurrent, no filesystem access, so thereconcileInventory: falsehot path is preserved. The explicitrequestedKeys.length === 0 → []guard correctly avoidsinArray(..., [])widening to the whole company.- The catalog lookup is gated on an
absentclassification actually existing, so the happy path and theunresolved_source-only path pay nothing, and it is wrapped in atry/catchthat logs and degrades to the pre-change classification rather than failing run setup. That is the right failure posture for something that only refines warning wording. catalogSkillKeysbeing optional, with omission reproducing the old behavior exactly, keeps the change additive — and there is a test pinning that (:162).- The test set covers the discriminator in both directions, not just the bug: a no-catalog-row key still classifies
absent(:150),unresolved_sourceis not shadowed for a surviving entry (:171), and:181pins the AC that reclassification never changes the reported set or its order. The integration case earning its keep by proving the call site consults the catalog — which the pure-function tests cannot — is the right instinct, as is asserting the "no row at all" control key alongside it so the test cannot pass by relabelling everything. - Counts are demonstrably untouched by the reclassification, asserted at both the notice layer and the context-snapshot layer.
Recommended Action
- No Critical issues — nothing blocks on correctness of the classification itself.
- Address the two Important items this cycle: the self-healing claim over a class that includes permanent faults is the substantive one, and the mangled test line is a one-line revert.
- Consider the two Suggestions opportunistically.
…ealing Review follow-up on #1679 (BLO-31993). `resolveRuntimeSkillSource` returns `null` only when materialization throws, and `.catch(() => null)` discards which throw it was. Two causes land at the classification seam byte-identically: - a rolling sweep caught mid-flight, which a later run clears on its own; and - `materializeRuntimeSkillFiles` throwing deterministically at the `!wroteSkillFile` guard because no `SKILL.md` content can be produced, which throws identically on every subsequent run. The notice asserted the first ("that clears on its own, so a later run will pick them up"), so a permanently-unmaterializable skill was told to wait forever — the same false-trigger shape as the bug this PR fixes, sign flipped. State what is known, not what is inferred: - rename the reason `materialization_pending` -> `runtime_files_unpublished` so the enum names the observed state rather than asserting a cause; - reword the remediation to give both causes and say to report a persistent one, keeping the load-bearing half ("already in the library, do not import them again"), which holds for both. Also from the review: - annotate the truncated-list overflow line with how many hidden keys are pending, so every remediation paragraph has a visible referent. The flags stay derived from the full set deliberately: deriving them from the shown slice would assert "configuration fault ... retrying will not fix it" over hidden pending keys, reintroducing the bug being fixed. - restore the two-line form of an unrelated pre-existing test mangled by a line-join in the previous commit. - drop the discarded `.orderBy` in `listCatalogSkillKeys`; its only consumer feeds the result straight into a `Set`. The no-pending notice is still byte-identical to the pre-BLO-31993 paragraph. Control: restoring only the old wording (keeping the rename) fails the integration test on `not to contain 'so a later run will pick them up'`. Tests: heartbeat-unmaterialized-desired-skills 24 passed (+2), heartbeat-runtime-skills 5 passed, company-skills{,-service,-routes} 111 passed, server typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — both Important items were real; all four are addressed in Important 1 —
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0316571
Prior Findings Dispositioned (2)
- prior:60981c9 important 1 — fixed —
server/src/services/heartbeat.ts:2836-2841— the self-healing promise is gone and the enum is trigger-neutral.materialization_pendingwas renamed toruntime_files_unpublished(:2782-2783), and the remediation now states only what the seam can know: "What is known is only that their library row exists and their files are not on the runtime volume: that can be a materialization sweep still in flight, which a later run would clear on its own, or a skill whose stored copy cannot be materialized at all, which will not clear. Report it if it persists across runs." That is recommendation (a), and the doc comment at:2649-2661now names both throw paths explicitly rather than asserting the transient one. The integration test that induced the permanent branch also flipped its assertion to match (heartbeat-runtime-skills.test.ts:480-481:not.toContain("so a later run will pick them up"),toContain("Report it if it persists across runs")), so the test that previously pinned the false claim now pins its absence. - prior:60981c9 important 2 — fixed —
server/src/__tests__/heartbeat-unmaterialized-desired-skills.test.ts:173-178— the collapsed line is restored to its original two-line form (it(...) => {/expect(computeUnmaterializedDesiredSkills({). The PR's file stats corroborate it independently: this file is now198+ 0-, i.e. the change no longer deletes any pre-existing line.
Critical Issues (0)
Important Issues (1)
-
[code / comments]
server/src/services/heartbeat.ts:2829-2830— in a mixed notice, the config-fault sentence describes the pending keys and tells them the opposite thing.The two remediation paragraphs are supposed to partition the reported keys, but they are scoped by prose description, and the two descriptions collide almost word for word:
:2829-2830(config fault): "The keys that are not in the library, or whose files are missing from the runtime volume, are a configuration fault, not a transient error — retrying will not fix them.":2837-2838(pending): "The keys whose runtime files are not published yet … their files are not on the runtime volume … Report it if it persists across runs."
"files are missing from the runtime volume" and "files are not on the runtime volume" name the same state to any reader. The intended referent of the first is
unresolved_source, whose bullet reads "library entry exists but its files are not on the runtime volume" (:2784) — but the pending bullet reads "in the company skill library, but its runtime files are not published yet" (:2782-2783), which is the same claim in different words.The
{pending, absent}combination is the sharp case, and it is the likeliest one in production — a genuinely un-imported key alongside one caught mid-sweep. There,missingcontains nounresolved_sourceentry at all, so the clause "or whose files are missing from the runtime volume" has no correct referent whatsoever, and the only key it resembles is the pending one that the very next sentence tells not to re-import and to report if persistent. The static string is emitted wheneverhasConfigFault && hasPending, regardless of which config-fault reasons are actually present.This is the same failure shape as BLO-31993 itself: a flat verdict asserted over a key it does not describe. The PR's own mixed-class test (
heartbeat-unmaterialized-desired-skills.test.ts:257-273) uses exactly{pending, absent}and assertstoContain("retrying will not fix them")— so it pins the ambiguous string rather than catching it.- Recommended: scope the clause by the bullet labels the reader can actually see, rather than by paraphrase — e.g. build it from the reasons present, so a
{pending, absent}notice says "The keys marked not in the company skill library …" and only adds the runtime-volume clause when anunresolved_sourcekey is inmissing. Then extend the mixed test with a third case that asserts the runtime-volume clause is absent from a{pending, absent}notice.
Suggestions (3)
- [code]
server/src/services/heartbeat.ts:2807-2812— the truncation disclosure is one-sided. The comment at:2800-2805states the invariant well ("a truncated list has to say what it is hiding — otherwise a paragraph can describe a class with no visible referent"), buthiddenPendingonly discloses hiddenruntime_files_unpublishedkeys. The mirror case is unhandled: 20 visible pending keys plus 2 hiddenabsentones still setshasConfigFault, so the config-fault paragraph renders with no visible referent — the exact condition the comment names. Counting hidden config-fault keys too (or just naming both counts on the overflow line) would complete the stated invariant. Narrow in practice, since it needs more thanUNMATERIALIZED_SKILL_NOTICE_MAX_KEYS = 20unmaterialized keys. - [type-design]
server/src/services/heartbeat.ts:2822—hasConfigFaultis a negative predicate (reason !== "runtime_files_unpublished"), so any reason added to the union later is silently enrolled in the "configuration fault … retrying will not fix it" bucket by default. Given that this PR exists precisely because a flat verdict was asserted over a class it did not fit, an allowlist (reason === "absent" || reason === "unresolved_source") fails safe instead: a new reason would render no verdict rather than a wrong one, andtscwould not flag either form. - [tests]
server/src/__tests__/heartbeat-unmaterialized-desired-skills.test.ts:276-299— the truncation test assertsnot.toContain("a/b/pending-one")for the first hidden pending key but never fora/b/pending-two(:285). Asserting both (or asserting the rendered bullet count) makes the test fail if the cap is ever raised past 21, which is the boundary it is guarding.
Strengths
- Both Important findings from the previous head were addressed at the level of the argument, not the assertion. The rename to
runtime_files_unpublishedremoves the transience claim from the enum itself, so the name can no longer drift back into promising self-healing — that is a stronger fix than rewording the notice alone. - The
.catch(() => null)information loss is now documented at the seam where it matters (:2649-2661), naming both the atomic-sweep race and the deterministicSKILL.mdthrow, and explaining that the classification is named for the observed state rather than a cause. That is the right conclusion to draw from an irreducibly ambiguous signal, and it is written where the next reader will hit it. - The integration test comment (
heartbeat-runtime-skills.test.ts:387-407) says out loud that it induces the permanent branch deliberately, because that is the case a transient-sounding notice would mislead. Choosing the adversarial branch for the fixture, and stating why, is what makes the test load-bearing rather than decorative. - The
absent-only control key alongside the pending key means the integration test cannot pass by relabelling everything, and:114pins that a catalog-free call reproduces pre-change behavior exactly — so the additive claim is tested, not just asserted. - Prior Suggestion 2 was taken:
listCatalogSkillKeys(company-skills.ts:5417-5438) no longer sorts a result whose only consumer feeds it into aSet. TherequestedKeys.length === 0 → []guard againstinArray(..., [])widening to the whole company is still there and still correct. - The catalog lookup remains gated on an
absentclassification existing, wrapped intry/catch, and degrades to the pre-change classification with a logged reason — the right failure posture for something that only refines warning wording.
Recommended Action
- No Critical issues.
- Address the one Important item this cycle: in a
{pending, absent}notice the config-fault clause currently describes the pending keys and gives them the opposite advice, which is the failure shape this PR exists to remove. - Consider the three Suggestions opportunistically.
Note: mergeStateStatus is BEHIND at this head; the branch will need updating before merge. reviewDecision is empty, so no required-review protection is unmet on this PR.
Review follow-up on #1679 at head 0316571. In a mixed notice the config-fault sentence was scoped by a paraphrase -- "the keys that are not in the library, or whose files are missing from the runtime volume". In the {runtime_files_unpublished, absent} combination -- the likeliest one in production -- `missing` holds no `unresolved_source` entry at all, so that second clause has no referent, and the only key it resembles is the pending one the very next sentence gives the opposite advice to. That is this bug's own shape one level up: a flat verdict asserted over a key it does not describe. The verdict now quotes the bullet labels the reader can actually see, built from `UNMATERIALIZED_SKILL_REASON_SUMMARY` so it cannot drift from the rendered bullets, and only names a class that is actually reported. Also taken from the same review: - `hasConfigFault` was a negative predicate, so any reason added to the union later would be silently enrolled in "retrying will not fix it". Replaced with an allowlist: a new reason renders no verdict rather than a wrong one. `tsc` flags neither form, so the safety has to come from the shape of the predicate. - The truncation disclosure was one-sided -- it named hidden pending keys but not hidden config-fault ones, so 20 visible pending keys plus one hidden `absent` rendered the config-fault paragraph with no visible referent. Same defect mirrored; both directions now disclosed, and only in a mixed notice, since a single-class notice already describes its own hidden keys. - The truncation test asserted only the first hidden pending key; it now asserts both, so it fails if the cap is raised past 21. The no-pending path is untouched and still reproduces the pre-BLO-31993 paragraph byte for byte (pinned by an existing assertion). Control: reverting only the source change fails all 3 new tests and leaves the 24 pre-existing ones green, so the additive claim is tested. heartbeat-unmaterialized-desired-skills 27 passed; heartbeat-runtime-skills 5 passed (embedded pg); server typecheck clean.
…fix/blo-31993-materialization-pending-reason
Disposition of review
|
| check | result |
|---|---|
heartbeat-unmaterialized-desired-skills |
27 passed (+3) |
| ↳ source reverted (control) | 3 failed / 24 passed |
heartbeat-runtime-skills (embedded pg) |
5 passed |
| both suites, post-merge | 32 passed |
tsc --noEmit (server), post-merge |
clean |
heartbeat-unmaterialized-desired-skills.test.ts remains +0 - on pre-existing lines, so the earlier line-join finding stays fixed.
One note for anyone reproducing locally: after merging master, the server typecheck reports Property 'metricLabels' does not exist on 'PaperclipPluginManifestV1'. That is a stale @paperclipai/shared dist, not a real error — the field is in packages/shared/src/validators/plugin.ts:732. pnpm --filter @paperclipai/shared build clears it.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 20e2828
Prior Findings Dispositioned (1)
- prior:0316571 important 1 — fixed —
server/src/services/heartbeat.ts:2857-2872— the config-fault verdict no longer describes the pending keys. The clause is now derived fromconfigFaultReasons(:2813-2815, the reasons actually present inmissing) and quotesUNMATERIALIZED_SKILL_REASON_SUMMARYlabels verbatim viacovered(:2865-2867) instead of paraphrasing the state. In the sharp{runtime_files_unpublished, absent}caseconfigFaultReasonsis exactly["absent"], so the sentence renders as "The keys marked *not in the company skill library* are a configuration fault…" and the unreferented paraphrase "or whose files are missing from the runtime volume" is gone from the source entirely. The new test atheartbeat-unmaterialized-desired-skills.test.ts:181-200pins both halves — the verbatim-label verdict andnot.toContain("whose files are missing from the runtime volume")— and:202-216pins the two-label join, so the previously-ambiguous string is now asserted absent rather than asserted present.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[type-design]
server/src/services/heartbeat.ts:2840— the allowlist adopted 27 lines above is not applied at the second site, so the invariant its comment asserts is not the one the code holds.UNMATERIALIZED_SKILL_CONFIG_FAULT_REASONS(:2797) exists precisely so that "a reason added to the union later renders no verdict instead of being silently enrolled inretrying will not fix it" (:2789-2796). ButhiddenConfigFaultis computed ashidden.length - hiddenPending— the same negative derivation, by subtraction rather than by!==. A fourth reason that is neitherabsentnorunresolved_sourcewould be counted into the overflow disclosure as "N a configuration fault", which is the enrollment the allowlist was introduced to prevent, just one paragraph lower. Correct as of this head (three reasons, one pending), so this is latent rather than live. Filtering onUNMATERIALIZED_SKILL_CONFIG_FAULT_REASONShere too would make the two sites agree and make the:2789-2796rationale true of the whole function. -
[code]
server/src/services/heartbeat.ts:2816—isMixedgates the disclosure onhasPending, but the config-fault paragraph became label-scoped in this same commit, and the gate was not widened to match. The comment at:2831-2835justifies the gate on the grounds that "when every reported key is one class, the single paragraph already describes the hidden ones too" — which held while the paragraph named a class. Now it enumerates labels: with no pending key, 20 visibleabsentkeys and 2 hiddenunresolved_sourceones,configFaultReasonsis["absent", "unresolved_source"], so the paragraph names*library entry exists but its files are not on the runtime volume*while no bullet carries that label andisMixedisfalse, so nothing discloses it. Same shape as the mirror case fixed at this head, one level finer: a label with no visible referent. Gating on "more than one distinct reason present inmissing" rather than onhasPendingwould cover it. A related nit in the same block: the disclosure wording is class-level ("2 a configuration fault") while the paragraph is label-level ("marked *not in the company skill library*"), so bridging them costs the reader an inference — reusing the same labels in the disclosure would close both points at once. Both need more thanUNMATERIALIZED_SKILL_NOTICE_MAX_KEYS = 20unmaterialized keys, so this is narrow in practice.
Strengths
- The Important finding was fixed at the level of the mechanism rather than the string. Deriving
coveredfromUNMATERIALIZED_SKILL_REASON_SUMMARYmeans the verdict and the rendered bullets cannot drift apart later: changing a bullet label now changes the verdict that quotes it, in the same edit. That is a stronger fix than rewording the sentence, which would have left the two free to diverge again. - The allowlist at
:2789-2797carries its own rationale, and the rationale is the right one —tscflags neither the negative predicate nor the allowlist, so the safety has to come from the shape of the predicate, and "render no verdict" is the correct default for a change that exists because a verdict was rendered over a class it did not describe. Suggestion 1 above is that this reasoning is not yet applied everywhere, not that it is wrong. - The
absent-only gate on the catalog lookup (:27482) is exactly the right predicate, and provably so:runtime_files_unpublishedcan only ever arise from reclassifying anabsentkey, because the first pass runs catalog-free, so every no-entry key startsabsent. Anunresolved_sourcekey already knows its library row exists and needs no lookup. So the gate cannot skip a key that would have been reclassified, and the happy path pays nothing. - The failure posture on the lookup is right:
try/catchthat degrades to the pre-change classification, logserrwith company/agent/run ids, and states in the comment why it must never fail run setup. A warning-wording refinement should not be able to break a run, and it cannot. listCatalogSkillKeys(company-skills.ts:5417-5438) stays a bare key projection — noresolveRuntimeSkillSource, noensureSkillInventoryCurrent, no filesystem — so thereconcileInventory: falsehot path is genuinely preserved rather than nominally preserved, and the docstring says which constraint that protects. TherequestedKeys.length === 0 → []guard againstinArray(..., [])widening to the whole company is still present and still correct.- The two new truncation tests are mirror images and both assert the absence of the visible key, not just the presence of the disclosure, so neither can pass by rendering everything. Asserting both
pending-oneandpending-two(:235-238) makes the pair fail if the cap is raised past 21, which is the boundary they exist to guard — the earlier one-sided version would have silently stopped testing it. - The reverted-source control (3 new tests fail, 24 pre-existing stay green) is the load-bearing verification claim, and it is the right one to make: it establishes that the new assertions catch the defect rather than passing by construction, and that the change is additive.
heartbeat-unmaterialized-desired-skills.test.tsremaining+269 -0independently corroborates that no pre-existing line was touched, so the earlier line-join finding stays fixed.
Recommended Action
- No Critical issues.
- No Important issues — the sole active prior finding is fixed at the mechanism, and nothing new rises to blocking.
- Consider the two Suggestions opportunistically. Both are the same theme as the finding just fixed — a verdict or disclosure whose referent the reader cannot resolve — surviving in the two narrow corners the fix did not reach, and both are latent rather than live at this head.
Note: mergeStateStatus is BEHIND at this head, so the branch needs updating before merge. reviewDecision is empty, so there is no required-review protection unmet on this PR.
The truncation disclosure counted hidden config-fault keys as `hidden.length - hiddenPending` — the same negative derivation that UNMATERIALIZED_SKILL_CONFIG_FAULT_REASONS was introduced to replace, one paragraph lower. A reason added to the union later would have been silently counted here as "a configuration fault" while the verdict, reading the allowlist, correctly declined to cover it: the two sites disagreeing about the same question. Counting per reason off the allowlist also lets the disclosure quote the same labels the verdict does, so the reader matches "N <label>" against a label visible on a bullet instead of bridging a class word to a label by inference. Co-Authored-By: Claude <noreply@anthropic.com>
|
Both Suggestions addressed at Taken —
|
| check | result |
|---|---|
heartbeat-unmaterialized-desired-skills |
29 passed (+2 new) |
| ↳ same tests, source reverted only (control) | 2 failed / 27 passed |
tsc --noEmit (server) |
clean for the diff |
The control is the load-bearing half: reverting only heartbeat.ts fails exactly the two new assertions and leaves all 27 others green, so they test the change rather than restating it. The third new test passes on both sides by design — it pins behaviour that is already correct, which is the whole point of declining rather than "fixing" it.
One note on the red checks at the previous head, so it is not re-diagnosed: General tests (workspaces-a) failed on company-import-export-e2e.test.ts with Could not reach the Paperclip API … fetch failed — the CLI could not reach its locally-spawned server. verify then failed purely as its downstream (Upstream lane(s) reported failure: general_tests). This diff is confined to server/src/{services,__tests__} and cannot affect whether a CLI e2e test reaches a local API; master is green on the same lane. Re-running with this push.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f95d6db
The sole change to PR-owned files since the last head is 9569da89, which takes prior Suggestion 1: hiddenConfigFault is now derived per reason from UNMATERIALIZED_SKILL_CONFIG_FAULT_REASONS (heartbeat.ts:2849-2851) instead of by subtraction, so the disclosure and the verdict answer the same question from one list. All prior Critical/Important findings were dispositioned fixed at earlier heads and the previous review minted none, so there is no active prior-finding set and no disposition section below. The remaining commits in the range came in via the master merge; the branch is now behind_by: 0, clearing the standing BEHIND note.
Critical Issues (0)
Important Issues (1)
-
[tests]
server/src/__tests__/heartbeat-unmaterialized-desired-skills.test.ts:402— the third assertion in the new allowlist guard can never fail.expect(notice).not.toContain("a configuration fault, not a transient error — retrying will not fix them.\n");
The trailing
\nmakes this unconditionally true.buildUnmaterializedSkillNoticeMarkdownspace-joins the remediation sentences (heartbeat.ts:2910,remediation.join(" ")) and places that single joined string as the last element of the outer\n-join (:2903-2912), so no remediation sentence is ever followed by a newline — in any input shape, not just this fixture.I rendered the function against this test's exact fixture to check rather than infer:
[1] verdict sentence PRESENT in rendered notice : true [2] assertion string (target + newline) present : false [3] char actually following the verdict sentence: " " [5] reasons=absent -> target+newline: false reasons=runtime_files_unpublished -> target+newline: false reasons=absent|runtime_files_unpublished|unresolved_source -> target+newline: falseLine [1] is the substantive point: the verdict sentence is in the notice, and correctly so —
a/b/goneis genuinelyabsent, soconfigFaultReasonsis["absent"]and the label-scoped verdict must render. The intent stated in the comment above it — "the unknown reason … never described by a verdict that does not cover it" — therefore cannot be expressed as an absence check on that sentence at all; the sentence is supposed to be there. Appending\nis what makes the assertion pass, and it inerts it.The guard itself is not lost:
:398(- …and 2 more (1 not in the company skill library)) and:401(not.toContain("2 not in the company skill library")) both do real work, and I confirmed both evaluate as the test expects. So this is a dead assertion inside a live test, not a coverage hole. It matters because it reads as an extra guard on exactly the invariant this commit exists to protect, and a reviewer or a later editor will count it as one — the same shape as the defects this PR has been removing, in the test layer: a claim whose referent does not hold.- Recommended: drop
:402, or replace it with an assertion that can distinguish pass from fail. The intent is already fully covered by:398+:401; if a third is wanted,expect(notice).toContain("The keys marked *not in the company skill library* are a configuration fault")pins that the verdict names only the label the allowlist admits, which is the claim the comment is reaching for.
- Recommended: drop
Suggestions (2)
-
[type-design]
server/src/services/heartbeat.ts:2885— the allowlist is now applied at two of three sites; the flat-verdict branch is the third.configFaultReasons(:2813) scopes the mixed verdict andhiddenConfigFault(:2849) now scopes the disclosure, but whenhasPendingisfalsethe else-branch emits"This is a configuration fault, not a transient error — retrying will not fix it."over every reported key, unscoped. With a fourth reason added to the union that is neitherabsentnorunresolved_sourcenor pending, that sentence enrolls it — precisely the enrollmentUNMATERIALIZED_SKILL_CONFIG_FAULT_REASONSwas introduced to prevent, now the only remaining place it can happen. Not live at this head: with three reasons,hasPending === falseimplies every key is a config fault, so the flat verdict is correct today. Worth noting that the new test at:411-428pins the flat-verdict behavior deliberately ("a later widening of that gate has to argue with a test rather than a comment"), so if a fourth reason is added that test will keep passing while the sentence over-claims. Gating the flat branch onconfigFaultReasons.length === UNMATERIALIZED_SKILL_CONFIG_FAULT_REASONS.length && missing.every(e => …)— or simply always using the label-scoped form — would close it. -
[comments]
server/src/services/heartbeat.ts:2854— the new rationale is half-realized on the pending side. The comment at:2839-2848justifies the per-reason counting partly on the grounds that it "lets the disclosure name the same labels the verdict quotes, so the reader matches 'N <label>' against a label they can see on a bullet instead of bridging a class word to a label by inference." That now holds for the config-fault buckets, which map throughUNMATERIALIZED_SKILL_REASON_SUMMARY(:2855-2857). The pending disclosure one line above still uses a hand-written paraphrase,"N with runtime files unpublished", against a bullet reading"in the company skill library, but its runtime files are not published yet". Close enough that a reader will bridge it, so this is presentation rather than correctness — but it is the one string in the block the stated invariant does not cover, andUNMATERIALIZED_SKILL_REASON_SUMMARY.runtime_files_unpublishedis already in scope.
Strengths
- The prior suggestion was taken at the mechanism, not the symptom. Replacing
hidden.length - hiddenPendingwith a per-reason count off the allowlist means the disclosure and the verdict now derive from the same list, so they cannot answer "which class are the hidden keys?" differently — which was the actual complaint, not the arithmetic. - The new test at
:380-403picks the adversarial fixture and says why: theas unknown as UnmaterializedDesiredSkillcast is called out in-comment as the point, becausetsccannot catch the enrollment and so the guard has to live in the shape of the derivation and be pinned by a test. That is the correct reading of what the allowlist is for, and testing a reason that does not exist yet is the only way to test it. - The second new test (
:411-428) is a rebuttal of a prior Suggestion rather than a compliance change, and the rebuttal is right. I had claimed that 20 visibleabsentkeys plus hiddenunresolved_sourceones would name a label with no visible bullet; the label-scoped verdict is gated onhasPending, so with nothing pending the flat sentence renders and no label is named. Disagreeing with a review finding and pinning the disagreement with a test — so a later widening of the gate has to argue with an assertion rather than a comment — is a better outcome than complying with it. - The discriminator holds at the SQL layer, which is what makes the whole fix sound.
listCatalogSkillKeysfilters oneq(companySkills.companyId, companyId)(company-skills.ts:5427-5436) and thereconcileInventory: falsebranch oflistRuntimeSkillEntriesfilters on the identical predicate (:5444-5450). Same row set, so a key returned by the catalog projection provably had a row in the entries pass and was dropped byresolveRuntimeSkillSource— there is no soft-delete or status column on one side that could make "row exists" and "row was considered" diverge and turnruntime_files_unpublishedinto bad advice. The two queries cannot drift apart without someone changing both. listCatalogSkillKeysstays a bare key projection — noresolveRuntimeSkillSource, noensureSkillInventoryCurrent, no filesystem — so thereconcileInventory: falsehot path is preserved in fact and not just in intent, and the docstring names the constraint it is protecting. TherequestedKeys.length === 0 → []guard againstinArray(..., [])widening to the whole company is still present and still correct.- The call-site gate on an
absentclassification existing (heartbeat.ts:27495) remains exactly right, and provably so: the first pass runs catalog-free, so every no-entry key startsabsent, andunresolved_sourcealready knows its row exists. The gate cannot skip a key that would have been reclassified, and the happy path pays nothing. Thetry/catchdegrades to the pre-change classification and logserrwith company/agent/run ids rather than swallowing it — the right posture for something that only refines warning wording. - The
mastermerge is clean and touches onlyscripts/approve-paperclip-api-digest.*from the other parent; the four PR-owned files are unchanged by it, andheartbeat-unmaterialized-desired-skills.test.tsremains+328 -0, independently corroborating that no pre-existing line was disturbed by this sync.
Recommended Action
- No Critical issues.
- Address the one Important item this cycle: it is a one-line deletion or replacement in the test just added, and the invariant stays guarded either way.
- Consider the two Suggestions opportunistically. Both are the same theme as the finding
9569da89just fixed — a verdict or a label whose referent the reader cannot resolve — surviving in the last two corners the fix did not reach, and both are latent rather than live at this head.
CI at this head is still queued for the full matrix (Build, Typecheck + Release Registry, General tests, e2e); policy, Helm chart, and Vendored claude_k8s adapter have completed success. reviewDecision is empty, so no required-review protection is unmet on this PR.
Thinking Path
Linked Issues or Issue Description
reasonand remediation were not.qa-live-verificationdocument has the byte-level notice text and sweep timing.The live repro. An agent's run prompt at
2026-09-05T10:53:52Zcarried:Both had been in the library since 2026-07-21. A rolling sweep rewrote those two runtime entries at
11:03:54Zand11:04:22Z— after the prompt was built.Root cause.
computeUnmaterializedDesiredSkillsassignedabsentwhenever a declared key yielded no entry.listRuntimeSkillEntriesyields no entry in two cases: (1) nocompanySkillsrow, so the key never enters the filter loop; (2) a row exists butresolveRuntimeSkillSourcereturnednulland the caller dropped it with a barecontinue. (2) is what a sweep looks like from here —materializeRuntimeSkillFilesisrm -rf→mkdir→ per-file write, not atomic.What Changed
server/src/services/company-skills.ts— newlistCatalogSkillKeys(companyId, skillKeys?): a bare key projection overcompanySkills. NoresolveRuntimeSkillSource, noensureSkillInventoryCurrent, no filesystem access. An explicit empty request returns[]rather than widening to the whole company viainArray(..., []).server/src/services/heartbeat.ts—UnmaterializedDesiredSkill.reasongainsmaterialization_pending;computeUnmaterializedDesiredSkillstakes an optionalcatalogSkillKeysused only to split the no-entry branch.server/src/services/heartbeat.ts— the notice now builds its remediation from what is actually in the delta instead of asserting one verdict flatly, so the two classes get opposite advice.server/src/services/heartbeat.ts(AC2 injection site) — a first, catalog-free pass runs as before; the catalog lookup happens only if that pass produced anabsent, and is wrapped so a lookup failure logs and falls back rather than failing run setup.companySkillsrow.materialization_pending(new)absentsourceStatus: "missing"unresolved_sourceVerification
Run locally on this branch:
Both new tests were run against the pre-fix tree, and both fail there — 6 failed / 16 passed in the unit file, 1 failed / 4 passed in the integration file, with every pre-existing case still green. The pre-fix integration failure reproduces the reported defect exactly: the notice rendered
`<pendingKey>` — not in the company skill libraryfor a key with a live catalog row.That control matters because of why CI missed this: the old unit test constructed
runtimeSkillEntriesas the sole source of truth, so "key absent from the array" trivially meantabsent, and the production state "catalog row exists, inventory row does not yet" was unrepresentable. The integration case is the half that proves the call site consults the catalog — the pure function being correct does not establish the wiring.Reviewer checks worth making against the linked ACs:
reconcileInventory: false—heartbeat.tscall site; nothing added to thelistRuntimeSkillEntriesoptions.reports the same keys in the same order with and without catalog keys.keeps the import guidance for a genuinely absent key.Risks
Low risk. The change is confined to how one advisory prompt notice is classified and worded; it alters no control flow, no skill resolution, and no adapter contract.
absent, which requires a declared key to have already failed to materialize. Runs where every skill resolves are untouched and issue no extra query.try/catchwith alogger.warnand a fallback to the pre-change classification. Judgement call worth a reviewer's eye: refining a warning's wording should not become a new way to fail run setup, but it does mean a persistent DB fault would silently restore the old (wrong) wording — hence the log line rather than a silent swallow.UnmaterializedDesiredSkill["reason"]has no consumers outsideheartbeat.tsand its tests (verified by grep acrossserver/,src/,packages/). The rendering map is keyed byRecord<reason, string>, so a future reason is a compile error rather than a silent fallthrough.materializedCountis unchanged by construction — amaterialization_pendingkey produces no runtime entry, exactly asabsentdid, so the subtraction still counts onlyunresolved_source.paperclipUnmaterializedSkillsis persisted via the existingcontextSnapshot; a health sweep readingreasonwill now see a third value, which is the intended signal.Model Used
claude-opus-5, 1M context variant —claude-opus-5[1m]as configured on this agent), extended thinking, with tool use and code execution (Claude Code / Paperclipclaude_k8sadapter). All tests, typechecks and the pre-fix control runs above were executed, not predicted.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code