Fix code review findings: correctness bugs, broken lockfile, and duplication cleanup - #2
Merged
Merged
Conversation
pnpm-lock.yaml had no importer entry for packages/agent-manifest even though its package.json declares ajv/ajv-formats/yaml as dependencies, breaking `pnpm install --frozen-lockfile` in every CI workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
recordHumanDecision demotes a class immediately on a severe miss, but runLevelsEngine independently re-demoted it again on its next run while the same observation was still in the trailing window, turning "one severe miss drops two levels" into repeated 2-level drops. Track which severe misses already triggered a demotion and only demote for unapplied ones. Also fix agreementRate: `ledger.slice(-window)` with window=0 returned the whole ledger (Array.prototype.slice(-0) behaves like slice(0)), not an empty window as a class configured with window:0 would intend. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
The n-threshold check only ran when both figure.direction and figure.source were present, so a directional figure with no source skipped the check entirely regardless of how small n was — exactly the unblockable below-n directional claim R11 exists to prevent. A directional figure now requires a source so its threshold can be checked; omitting it is rejected at write instead of silently passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
… check queue.ts's demote() had no caller-identity check at all, unlike its sibling promote() — any caller could silently remove a slug from the ready queue with no named-human check, despite the module's own intent that no agent may set priority. demote() now requires the same human-approver check as promote(). The weekly expiry sweep still needs to prune a queue entry whose backing brief has expired with no human present; that's not a priority decision, so it goes through a new, narrowly-scoped pruneExpiredFromQueue() instead of bypassing demote()'s guard. The "looks like an agent, not a person" regex was duplicated verbatim between queue.ts and content-distributor's send-approval gate; both now share one looksLikeAgent() helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
sweepExpiredBriefs moved an expired brief's file to briefs/expired/ and resolved its Triage item, but never removed it from queue.yaml's ready array — queueHasRoom only counts array length, so a promoted brief that later expired would occupy a ready-queue slot forever. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
review.schema.json's own description asserts that a piece cannot be published with an incomplete record, but no invariant anywhere checked that a published document has a review record — checkBriefAndPack only checked for a brief and a pack. A document published outside the normal flow (e.g. directly via the Payload admin UI) could silently skip the calibration/shadow pipeline forever, since it treats review records as its sole input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
…not just the first outside.replace(c.text, ' ') is a non-global string replace — it only stripped the first occurrence of a claim's text. A carried claim repeated verbatim elsewhere in the adaptation (e.g. once in a hook, once in the body) left its second occurrence in the "outside claims" text, which the regulated-category check then wrongly flagged as unannotated. Use split/join to strip every occurrence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
…tion collectionFor mapped any surface that wasn't 'recipes' to 'posts' by default, so a landing or newsletter draft staged with no explicit collection override silently landed in the blog's posts collection instead of failing. Surfaces without a known CMS collection now throw at stage time instead of guessing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
tsconfig.json's include list covered src/**, scripts/**,
packages/*/src/**, and tests/**, but not agents/**/*.ts, even though
vitest.config.ts's test include explicitly covers agents/**/*.test.ts.
Large parts of the agents tree (including files reached only from
their own tests, never from scripts/** or packages/*/src/**) were
never type-checked by the CI-gating `pnpm typecheck` command.
Adding it surfaced two real errors, now fixed:
- planner.test.ts's withId() placed an explicit `id` before a spread
of a same-shaped object, so the spread silently overwrote it —
reordered so the explicit id wins, matching the helper's intent.
- studio.test.ts cast a general SerializedLexicalNode to an ad hoc
{ claimId: string } type, which TS correctly rejected as an unsound
assertion — replaced with a type-predicate filter using the already
exported SerializedClaimNode type.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
… just drafts The pre-publish gate suite's options.corpusSlugs came from artifacts.ts's corpusSlugs(), which only scanned .agency/content/drafts/, while content-monitor's run.ts independently defines the corpus as published documents plus drafts. A draft linking to an already-published page with no current draft file was wrongly flagged by the links gate as pointing at no known document. corpusSlugs now also reads .agency/content/published/, matching each document by its own slug field the same way content-monitor does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
On a gate crash, runGates fell back to gate.name (the JS function identifier, e.g. claimCoverage) instead of the gate's own canonical kebab-case id (claim-coverage) that the same gate returns on pass/fail. 3 of 7 gates (claimCoverage, styleLint, briefConformance) had mismatched names, so any lookup keyed on the canonical id — the pattern used throughout this test suite — silently failed to find a crashed gate's result. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
The identical `arg(name, fallback?)` closure reading `--<name> <value>` out of process.argv was reimplemented byte-for-byte in 10 files across scripts/**, agents/content-monitor/agent/run.ts, and packages/content-pipeline/src/cli.ts (calibrate.ts also carried a sibling `flag()` boolean-flag helper found nowhere else). Factored into packages/content-pipeline/src/cliArgs.ts, imported everywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
agentCannotPublish and agentCreatesDraftsOnly repeated the identical user-extraction, null-check, non-agent-passthrough, and _status === "published" denial prefix, differing only in what they return once an agent's write isn't an attempted publish. Factored into one agentPublishAttempt() helper both call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
The HEAD-then-GET-fallback, 10s-AbortController link resolution algorithm was duplicated near-verbatim between the pre-publish links gate and content-monitor's checkExternalLinks, and both copies shared the same bug: the abort timer was only cleared on the success path, so a thrown fetch error (DNS failure, connection refused) leaked a pending timer for up to 10s. Factored into one resolveExternalLink(), exported from gates/links.ts and reused by content-monitor, with the timer cleared in a finally block on every path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
…Slugs Each brief's gate suite is fully independent — including the network-bound external-link checks with up to a 10s timeout each — but the CLI ran them one at a time, so wall-clock time was the sum of every brief's gate time rather than the max. Also hoisted corpusSlugs(paths) (a directory listing) out of the per-slug loop; it was re-read from disk on every iteration even though it can't change mid-run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LX8hsAFrvvdgpFXwLDJJbY
…w-ms4h46 # Conflicts: # agents/content-planner/tests/planner.test.ts # packages/content-pipeline/src/calibration.ts # packages/content-pipeline/tests/calibration.test.ts
Content QA
|
| Gate | Status | Detail |
|---|---|---|
| structure | ✅ pass | — |
| claim-coverage | ✅ pass | — |
| prohibition | ✅ pass | — |
| style-lint | ✅ pass | — |
| links | ❌ fail | external link https://soilquality.org.au/factsheets/organic-carbon did not resolve (fetch failed) |
| readability | ✅ pass | Flesch reading ease 70.3 |
| brief-conformance | ✅ pass | — |
couldNotVerify — read this before approving:
- District-average paddock organic matter figures for The Branch specifically — NSW DPI publishes regional soil data but nothing at locality resolution; partial verification only via the state-wide mapping. Left out of the piece.
slow-roasted-highland-beef — ✅ all gates pass
| Gate | Status | Detail |
|---|---|---|
| structure | ✅ pass | — |
| claim-coverage | ✅ pass | — |
| prohibition | ✅ pass | — |
| style-lint | ✅ pass | — |
| links | ✅ pass | — |
| readability | ✅ pass | Flesch reading ease 75.2 |
| brief-conformance | ✅ pass | — |
couldNotVerify — read this before approving:
- Specific micronutrient figures for Highland cattle as a breed — Breed-specific nutrition studies are thin; the sourced comparison is grass- vs grain-finishing generally, not Highland-specific. The piece must not imply breed-specific numbers.
Contributor
Author
|
The This URL is a pre-existing source citation in
Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the findings from a full-codebase
/code-reviewpass (9 parallel finder agents, deduped and independently re-verified against source before fixing). 18 of 20 findings fixed; 2 deferred with reasons below. Every commit is a self-contained, behavior-preserving fix or refactor with typecheck + the full test suite passing after each one.Correctness fixes
pnpm-lock.yamlwas missing the workspace entry forpackages/agent-manifest, breakingpnpm install --frozen-lockfilein every CI workflow right now — regenerated.calibration.ts: a severe miss could demote a decision class twice (once immediately, once again on the nextrunLevelsEnginepass) instead of the documented one 2-level drop; also fixedslice(-0)silently returning the whole ledger instead of an empty window.source, however smallnwas — exactly the unblockable path R11 exists to prevent.queue.demote()had no human-approver guard, unlike its siblingpromote(). It now requires a named human; the weekly expiry sweep uses a new, narrowly-scopedpruneExpiredFromQueue()for its unattended cleanup instead of bypassing the guard.published-has-reviewinvariant thatreview.schema.jsondocuments but nothing checked.adapt.ts: a non-global.replace()only stripped the first occurrence of a repeated carried claim, producing false compliance failures.stage.ts:collectionForsilently defaulted any non-recipessurface to thepostsPayload collection; it now fails loudly for surfaces with no known collection.tsconfig.jsonnever coveredagents/**, sopnpm typechecksilently missed real errors — added it and fixed the two errors it surfaced.corpusSlugs()only reflected staged drafts, so the pre-publish links gate disagreed with content-monitor's broader published+drafts corpus definition — reconciled.claimCoverage) instead of the gate's canonical id (claim-coverage) on 3 of 7 gates, breaking any lookup keyed on the canonical name.Duplication / efficiency cleanup
looksLikeAgent()helper (was duplicated verbatim across the queue and send-approval gates).arg()/flag()CLI parser, deduping 10 identical copies.agentPublishAttempt()predicate out ofagentCannotPublish/agentCreatesDraftsOnly.resolveExternalLink()used by both the links gate and content-monitor, fixing anAbortControllertimer leak present in both copies.Findings Deferred
social-postwrite is declared as a human-approved consequential write in the manifest, but noSocialClient/approval-gated implementation exists yet (unlikeemail-send). Left as-is rather than inventing a client with no real usage to gate — flagging so the eventual implementation routes through anassertApproved-style gate from the start.packages/agent-manifestandpackages/content-pipeline— sharing it needs an architecture decision (new shared package, or a dependency between two currently-independent packages), which is out of scope for a targeted fix pass.Test plan
pnpm typecheckclean after every commitpnpm test(133 tests) green after every commitpnpm gates,pnpm agents list, andscripts/queue/promote.tsafter the CLI refactors🤖 Generated with Claude Code
Generated by Claude Code