Harness evolution: approved blueprint, phase plans, and full Phase 1-4 implementation - #42
Harness evolution: approved blueprint, phase plans, and full Phase 1-4 implementation#42noodlemind wants to merge 14 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSummaryThe change adds branch-local knowledge layers, Git provenance, lifecycle commands, structural indexing, policy-v2 severities, structural verification, documentation, and integration tests. ChangesHarness evolution
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Design documentation only — no CLI, store, or primitive behavior changes. Maps nine externally supplied proposals (two-layer golden/branch-local knowledge, deterministic retrieval, structural indexing, layer-aware compounding, plan/verify enrichment, knowledge lifecycle commands, worktree hardening, token budgeting, provenance governance) onto the existing harness architecture, marking already-satisfied proposals as verify-and-document and phasing the rest. Two owner-fixed decisions are normative in the blueprint: branch-local knowledge layers extend the existing external store at ~/.harness/knowledge/<repo-id>/ (never an in-repo .harness/knowledge/), and structural parsing adopts web-tree-sitter as an optional WASM tier behind the existing extract seam with the lexical extractor as the permanent fallback. The capability registry is deliberately untouched: the blueprint ships with its Human Decision pending, and registry candidate entries are the first post-approval step. Current-state docs gain only clearly labeled planned-evolution pointers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0169cWqYhLedkYhL8hEyudSq
f809f7d to
ef9aeff
Compare
Adds a section committing the blueprint's proposed command surfaces (knowledge lifecycle, structural index) to be conforming citizens of the separately planned CLI workbench/TUI track: registry-dispatchable, versioned JSON envelope with JSONL streaming, cooperative cancellation with distinct cancelled/timed-out outcomes, and a stable generation stamp the workbench can render. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0169cWqYhLedkYhL8hEyudSq
…eprint Blueprint section 9 previously required only a human rendering and the workbench's JSON envelope for new command surfaces. This names the third audience — the agent — as a first-class rendering: budgeted, deterministic, injection-framed text following the existing context-pack/plan-slice/get pattern, with the rule that agents consume the budgeted lane and envelope JSON never enters model context. The structural query surface (P3) now points to that contract for its agent rendering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0169cWqYhLedkYhL8hEyudSq
Merges the owner's tightened draft with fixes for the findings from the three-lens adversarial review. Keeps the draft's improvements: repo-keyed structural index at ~/.harness/index/<repo-id>/structural/, the shadowed-claim promotion rule, branch rename handling, and the pinned TS/JS+Python+Java grammar set. Closes the review's design-soundness findings as normative clauses: protected golden claims can never be shadowed on the read path and the governance ledger binds both layers; promotion runs in an explicit lane (per-bucket ledgers, hash-verified candidacy exemption, never-strike rejections, chunked --all); the absorb-branch ledger action can never become an id's standing decision; hand-edit absorption, purge, rebuild, and mirroring get layer-aware semantics in section 5a; provenance fields must be emitted by both serializers; default-branch determination fails closed to branch-local; WASM grammars gain integrity verification; Phase 1 is reduced to reads-only so it is internally consistent; the events pre-work claim is corrected to hygiene. Restores section 9 (workbench relationship and the three-audience output contract). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0169cWqYhLedkYhL8hEyudSq
Records the Human Decision (approved 2026-08-06) in the evolution blueprint with the section 4/5/5a normative clauses as binding conditions. Adds the live Phase 1 plan (provenance stamps, events allow-list fix, branch detection and key derivation, gated layered read path, knowledge status) as a dated, schema-v1, gate-ready plan — validated green by harness validate-plan — plus undated Phase 2-4 drafts that respect the one-live-plan rule and are promoted to dated plans when their phase starts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0169cWqYhLedkYhL8hEyudSq
noodlemind
changed the title
…verlay, promotion lane, and lifecycle commands
… --since diffs, and doctor S1
…xpectations check
…ion-proposals-tqgkw6
noodlemind
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/harness/lib/knowledge/consolidate.mjs (1)
288-310: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRead consolidation aggregates from the routed layer
On a branch lane,
consolidateStatusreadslistLearnings(dir)whileapplyOpsvalidates againstlayerRoot. UselistLearnings(view.layerRoot)forlearnings,domains, andpromotionCandidatesso the status packet matches the bucket write target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/consolidate.mjs` around lines 288 - 310, Update consolidateStatus to read learnings via listLearnings(view.layerRoot) instead of listLearnings(dir), ensuring active counts, domainPressure, and promotionCandidates reflect the routed layer and match applyOps’ bucket write target.
🟡 Minor comments (16)
packages/harness/lib/flags.mjs-192-193 (1)
192-193: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a missing
--sincevalue.
harness index --structural --sincesetsflags.sincetoundefined.cmdIndexthen skipsvalidateSinceRef()and performs a full index. Reject a missing value before assigning the flag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/flags.mjs` around lines 192 - 193, Update the --since argument handling in the flags parser so the separate `--since` form validates that `argv[++i]` exists before assigning `flags.since`; reject the missing-value case instead of storing undefined, while preserving the existing `--since=<value>` behavior.packages/harness/lib/flags.mjs-203-204 (1)
203-204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a complete integer for
--stale.
parseInt('30days', 10)returns30. A malformed--stalevalue can therefore prune buckets by an unintended age. Validate the complete string, then require a safe positive integer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/flags.mjs` around lines 203 - 204, Update the stale-value parsing used by the --stale branches to validate the entire input as an integer rather than accepting parseInt prefixes such as “30days”, then require the result to be a safe positive integer. Preserve the existing flag assignment and error-label behavior in both --stale= and separate-argument forms, using parsePositiveInt as the shared validation point.packages/harness/lib/commands.mjs-373-400 (1)
373-400: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when a non-dry-run structural index does not persist.
buildStructuralIndexreturnswritten: falsewhen a contained write is refused. This command still writes a successful event, emitspass: true, and returns exit code 0. A caller can therefore treat an unavailable structural index as current.Return a failed result when
!flags.dryRun && !result.written. Keep dry-run successful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/commands.mjs` around lines 373 - 400, Update the result handling after buildStructuralIndex so a non-dry-run with result.written false is treated as failed: emit a failed event and JSON summary with exitCode 1, and return the corresponding failure result. Preserve successful behavior for persisted indexes and keep dry-run executions successful even when written is false.knowledge/proposals/harness-evolution-blueprint.md-3-8 (1)
3-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale proposal status.
Lines 3-8 say that implementation is prohibited pending a Human Decision. Lines 407-416 record approval on August 6, 2026. This conflict can cause plan or review tooling to treat the shipped work as unauthorized.
Proposed fix
-Status: **proposal — pending Human Decision.** -Design documentation only. Nothing in this document describes current behavior, and no -CLI, store, or primitive change may be built from it until the `## Human Decision` -section records an approval ... +Status: **approved.** +The `## Human Decision` section approved this blueprint on 2026-08-06. Its listed +conditions remain binding on implementation phases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge/proposals/harness-evolution-blueprint.md` around lines 3 - 8, Update the proposal status and prohibition text at the document header to reflect the approval recorded in the Human Decision section (August 6, 2026). Remove the stale pending/unapproved implementation restriction while preserving the document’s design-only scope and decision-handling references.docs/MEMORY-MODEL.md-708-709 (1)
708-709: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the documentation with the shipped implementation.
This PR ships Phase 3 structural indexing and Phase 4 per-check verification severity. The current statements describe both as planned or unshipped.
docs/MEMORY-MODEL.md#L708-L709: replace the unshipped statement with the current Phase 3 and Phase 4 status.docs/architecture/engineer-harness.md#L226-L226: update the link annotation to identify the blueprint as an approved design with shipped behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/MEMORY-MODEL.md` around lines 708 - 709, Update docs/MEMORY-MODEL.md lines 708-709 to state that Phase 3 structural indexing and Phase 4 per-check verification severity are shipped, replacing the unshipped-design wording. Update the link annotation in docs/architecture/engineer-harness.md line 226 to identify the blueprint as an approved design with shipped behavior.packages/harness/lib/structural/shape.mjs-24-26 (1)
24-26: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCorrect the documented
extractorTiervalues.Line 26 documents
"extractorTier": "lexical|tree-sitter". The builder writes'treesitter'without a hyphen. SeesanitizeEntryinpackages/harness/lib/repo-map/structural-index.mjs(line 164) andcreateTreesitterExtract(tier: 'treesitter'). This file is the stated integration contract, so a consumer that matches on the documented spelling will never match a real index.📝 Proposed fix
-// "extractorTier": "lexical|tree-sitter", "grammarVersions": {} } +// "extractorTier": "lexical|treesitter", "grammarVersions": {} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/structural/shape.mjs` around lines 24 - 26, Update the documented extractorTier values in the meta.json contract comment to use the actual builder spelling “treesitter” instead of “tree-sitter”, matching sanitizeEntry and createTreesitterExtract while preserving the lexical value.packages/harness/lib/repo-map/treesitter-extractor.mjs-98-106 (1)
98-106: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
lock.runtimeinloadGrammarsLock.
loadGrammarsLockonly requireslock.grammars. Two consumers then dereferencelock.runtimewithout a guard:
- Line 165:
check('runtime', lock.runtime)readsspec.package.- Line 430:
findWasm(roots, lock.runtime.package, lock.runtime.file).If
grammars.lockis present but has noruntimeentry, both sites throwTypeError.grammarStatusis synchronous and is called by doctor S1, so the throw escapes there. Validate the key once at the single parse point instead.🛡️ Proposed fix
export function loadGrammarsLock({ lockPath = DEFAULT_LOCK_PATH } = {}) { try { const lock = JSON.parse(fs.readFileSync(lockPath, 'utf8')); if (!lock || typeof lock !== 'object' || !lock.grammars) return null; + const runtime = lock.runtime; + if (!runtime || typeof runtime.package !== 'string' || typeof runtime.file !== 'string') return null; return lock; } catch { return null; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/repo-map/treesitter-extractor.mjs` around lines 98 - 106, Update loadGrammarsLock to require a valid lock.runtime object alongside lock.grammars before returning the parsed lock; otherwise return null. Keep the validation at this single parse point so grammarStatus and findWasm can safely use lock.runtime without additional guards.packages/harness/lib/verify.mjs-240-253 (1)
240-253: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle
skippedas neutral in verify output.
cmdVerifytreats every status other thanpassedas a failure candidate. This displays a skipped check as a warning, includes it in failure counts, and can select it as the next fix target. Excludeskippedfrom these failure paths. The evidence JSON already preservesstatus;events.safeChecksdoes not.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/verify.mjs` around lines 240 - 253, Update cmdVerify’s status handling so checks with status 'skipped' are neutral: exclude them from warning output, failure counts, and next-fix-target selection while retaining passed and actionable failure behavior. Preserve the existing status in evidence JSON, and update events.safeChecks to retain each check’s status as well.packages/harness/lib/knowledge/admin.mjs-1100-1115 (1)
1100-1115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude bucket learnings in the
--yespreview count.Lines 1107-1115 add bucket learnings to
archived. The confirmation preview does not. Line 1050 computesarchivedPreviewfromlistLearnings(storePath)only, so the blocked message at Line 1056 under-reports how many learnings a rebuild resets once buckets exist. A human confirms a destructive reset against the wrong number.🐛 Proposed fix for the preview count
Apply outside the selected range, at Line 1050:
- const archivedPreview = fs.existsSync(storePath) ? listLearnings(storePath).length : 0; + const archivedPreview = fs.existsSync(storePath) + ? listLearnings(storePath).length + + listBuckets(storePath).reduce((n, b) => n + listLearnings(b.dir).length, 0) + : 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/admin.mjs` around lines 1100 - 1115, Update the archivedPreview calculation in the rebuild confirmation flow to include learnings from every bucket returned by listBuckets(dir), matching the archived count later computed from archivedLearnings plus archivedBranch. Keep the existing preview message and confirmation behavior unchanged while ensuring --yes reports the full destructive-reset count.packages/harness/lib/knowledge/prune.mjs-51-53 (1)
51-53: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject malformed
--stalevalues withoutparseIntcoercion.parsePositiveIntrejects missing and fully invalid values, but it accepts7junkas7and truncates1.5to1. The latter changes the cutoff and can prune buckets newer than the requested age. Parse the complete value as a finite integer, and validatestaleDaysat thepruneBucketsboundary for direct callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/prune.mjs` around lines 51 - 53, Update parsePositiveInt to reject trailing characters and fractional values by requiring the complete input to be a finite integer, while preserving rejection of missing or invalid values. Add equivalent staleDays validation at the pruneBuckets boundary so direct callers cannot bypass parsing and use malformed values.packages/harness/lib/knowledge/overlay.mjs-99-112 (1)
99-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA killed
git merge-basereports "not an ancestor" instead of "unverifiable".The documented contract states that
falsemeans verified not-an-ancestor, andnullmeans unverifiable.spawnSyncsetsres.erroronly for spawn failures. If the 10 s timeout kills the process,res.erroris undefined on some Node versions andres.statusisnull, sores.status === 0 ? true : falseyieldsfalse.
loadLayeredLearningsthen drops the whole bucket from the overlay on line 154, andknowledge statusreports the bucket as ancestry-excluded. Treat a non-numeric exit status as unverifiable.🐛 Proposed fix
if (res.error) return null; - return res.status === 0 ? true : false; + // A timeout kill (signal set, status null) is UNVERIFIABLE, never a + // proven unrelated history. + if (typeof res.status !== 'number') return null; + return res.status === 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/overlay.mjs` around lines 99 - 112, Update bucketAncestryOk to treat any non-numeric res.status as unverifiable by returning null, while preserving true for status 0 and false for numeric nonzero statuses. Keep the existing res.error and exception handling unchanged.packages/harness/lib/knowledge/layer.mjs-119-124 (1)
119-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
branchExistsover-matches remote refs with slashes.The remote test is
r.startsWith('refs/remotes/') && r.endsWith('/' + branch). A ref such asrefs/remotes/origin/release/mainsatisfies that test forbranch === 'main'.branchExiststhen returnstruefor a branch that does not exist.Two callers degrade silently. Doctor check K5 in
packages/harness/lib/doctor.mjsstops reporting the bucket as an orphan.migrateRenamedBucketat line 144 stops treating the bucket as a rename candidate. Match the remote form exactly instead: one remote-name segment, then the full branch name.🐛 Proposed fix for the remote-ref match
export function branchExists(workspace, branch) { if (!branch) return null; const refs = listRefs(workspace); if (refs === null) return null; - return refs.some((r) => r === `refs/heads/${branch}` || (r.startsWith('refs/remotes/') && r.endsWith(`/${branch}`))); + return refs.some((r) => { + if (r === `refs/heads/${branch}`) return true; + if (!r.startsWith('refs/remotes/')) return false; + const rest = r.slice('refs/remotes/'.length); + const slash = rest.indexOf('/'); + return slash > 0 && rest.slice(slash + 1) === branch; + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/layer.mjs` around lines 119 - 124, Update branchExists so the remote-ref check matches exactly one remote-name segment followed by the full branch name, rather than any ref ending with /branch. Preserve local refs/heads matching and null handling, while ensuring refs such as refs/remotes/origin/release/main do not match branch "main".packages/harness/lib/knowledge/overlay.mjs-189-199 (1)
189-199: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve per-entry layer attribution.
The protected-shadow path returns two entries with the same
id.cmdOrientstores layer data in an object keyed byid, so the branch entry overwrites the golden entry.harness reportthen uses that single value for its unique-ID layer split. Preserve both entries' layer identity in the event and report data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/overlay.mjs` around lines 189 - 199, Update the protected-shadow handling in the overlay merge flow and the downstream cmdOrient/report data structures so layer attribution is keyed by each unique merged entry, not only by the shared base id. Preserve separate golden and branch layer values for both entries, and ensure harness report uses those per-entry identities when splitting layers.packages/harness/test/structural-shape-compat.test.mjs-19-29 (1)
19-29: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNeutralize the ambient Git configuration in this fixture.
initRepoandcommitAllrungitwithout overridingGIT_CONFIG_GLOBALandGIT_CONFIG_SYSTEM. Every other new suite in this PR sets both to/dev/null, for examplepackages/harness/test/structural-index.test.mjslines 22-27.A developer's global configuration therefore leaks into this fixture. If
commit.gpgsignis true,commitAllfails or blocks on a passphrase prompt. Ifcore.hooksPathis set globally, the fixture runs those hooks.🛡️ Proposed fix
+const GIT_ENV = { ...process.env, GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_SYSTEM: '/dev/null' }; + function initRepo(ws) { fs.mkdirSync(ws, { recursive: true }); - execFileSync('git', ['init', '-q'], { cwd: ws }); - execFileSync('git', ['config', 'user.email', 't@t'], { cwd: ws }); - execFileSync('git', ['config', 'user.name', 't'], { cwd: ws }); + execFileSync('git', ['init', '-q'], { cwd: ws, env: GIT_ENV }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: ws, env: GIT_ENV }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: ws, env: GIT_ENV }); } function commitAll(ws, message) { - execFileSync('git', ['add', '-A'], { cwd: ws }); - execFileSync('git', ['commit', '-qm', message], { cwd: ws }); + execFileSync('git', ['add', '-A'], { cwd: ws, env: GIT_ENV }); + execFileSync('git', ['commit', '-qm', message], { cwd: ws, env: GIT_ENV }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/structural-shape-compat.test.mjs` around lines 19 - 29, Update the git invocations in initRepo and commitAll to neutralize both global and system Git configuration by setting GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to /dev/null, matching the environment setup used by the other structural test suites. Ensure every git command in these fixture helpers inherits those overrides.packages/harness/test/structural-shape-compat.test.mjs-63-69 (1)
63-69: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPass the impacted-file allowlist through
plan.sections.impactedFiles.
parseImpactedFilesignoresplan.body, so this test also reportsunplanned-symbol-change. Add the supported plan shape and assert that nounplanned-symbol-changefinding exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/structural-shape-compat.test.mjs` around lines 63 - 69, Update the test’s plan fixture to provide the impacted-file allowlist through plan.sections.impactedFiles instead of plan.body, matching the shape consumed by parseImpactedFiles. In the assertions for runStructuralExpectations, also verify that result.findings contains no finding with type unplanned-symbol-change while preserving the existing removed-symbol-with-callers assertion.packages/harness/test/treesitter-extractor.test.mjs-181-196 (1)
181-196: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the runtime through Node module resolution
The hard-coded path checks only
packages/harness/node_modules. Ifweb-tree-sitteris hoisted, the test skips both integrity assertions and still passes. UsecreateRequire(import.meta.url).resolve('web-tree-sitter/tree-sitter.wasm')so the test covers hoisted installations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/treesitter-extractor.test.mjs` around lines 181 - 196, Update the runtime source lookup in the test setup to resolve web-tree-sitter/tree-sitter.wasm via Node module resolution using createRequire(import.meta.url), rather than deriving a path from DEFAULT_LOCK_PATH. Keep the existing conditional copy and integrity assertions driven by the resolved runtime path so hoisted installations exercise the corruption checks.
🧹 Nitpick comments (24)
packages/harness/lib/repo-map/index.mjs (1)
60-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the inner
structuralscore variable.Line 66 declares
const structuralinside the map callback. Line 28 declares an outerstructuralthat holds the loaded structural index. The two names describe unrelated values. The current code works, but a later edit that reads the index inside this callback would silently read the score instead.♻️ Proposed rename
- const structural = f.importedBy * 2 + Math.min(f.symbols.length, 12); - return { ...f, score: queryScore * 5 + structural }; + const degreeScore = f.importedBy * 2 + Math.min(f.symbols.length, 12); + return { ...f, score: queryScore * 5 + degreeScore };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/repo-map/index.mjs` around lines 60 - 68, Rename the inner structural score variable in the map callback that builds scored to a distinct name, and update the score calculation to use it; leave the outer structural index unchanged.packages/harness/lib/policy.mjs (2)
67-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard
checkSeverityForagainst prototype-inherited ids.
checkSeveritiesis a plain object.policy?.checkSeverities?.[id]therefore resolves ids such asconstructor,toString, andvalueOftoObject.prototypemembers instead of falling through todefaultSeverity. Check ids reach this function from plan frontmatter (verification.requiredinrunVerify), so a check namedconstructoryields a function as its severity. The outcome still degrades to gating inresolveOutcome, so this is not a bypass, but the recorded severity is wrong.🛡️ Proposed fix
export function checkSeverityFor(policy, id, defaultSeverity = 'enforce') { - return policy?.checkSeverities?.[id] ?? defaultSeverity; + const configured = policy?.checkSeverities; + return configured && Object.hasOwn(configured, id) ? configured[id] : defaultSeverity; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/policy.mjs` around lines 67 - 70, Update checkSeverityFor to accept only own properties of policy.checkSeverities when resolving id; inherited Object.prototype names such as constructor, toString, and valueOf must fall through to defaultSeverity. Preserve the existing optional handling and configured own-property severities.
49-63: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
checks:is honored underversion: 1.Line 49 accepts version 1 or 2. Line 63 then calls
parseCheckSeveritiesfor every policy, including a v1 policy. The comment at lines 7-10 presentschecks:as a v2 schema addition. A repository that declaresversion: 1and addschecks:silently gets v2 severity behavior, so the version field stops describing the active schema.Decide one behavior and make it explicit: either ignore
checks:under v1, or document thatchecks:is version-independent and adjust the comment at lines 7-10.♻️ Option: gate on the declared version
- checkSeverities: parseCheckSeverities(policy, policyPath), + checkSeverities: policy.version === 2 ? parseCheckSeverities(policy, policyPath) : {},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/policy.mjs` around lines 49 - 63, Update the policy construction around parseCheckSeverities so checks: is only applied when the declared policy version supports the v2 schema; for version 1, ignore checks and preserve the existing default severity behavior. Keep version validation for versions 1 and 2 unchanged, and make the version-gated behavior explicit in the returned checkSeverities value.packages/harness/lib/knowledge/store.mjs (1)
492-501: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
PROVENANCE_SHA_RErejects SHA-256 object ids.Line 492 accepts exactly 40 hex characters. Git repositories initialized with
--object-format=sha256produce 64-character object ids. On such a repository,provenanceLinesdropscommit:andbase:silently, so learnings written there carry branch provenance but no commit or base.provenanceBytesstays consistent with that, so the byte-cap math does not break — only the recorded provenance is lost.Accept both widths if you intend to support those repositories.
♻️ Proposed change
-const PROVENANCE_SHA_RE = /^[0-9a-f]{40}$/; +// Both git object formats: 40-hex (SHA-1) and 64-hex (SHA-256, git >= 2.29). +const PROVENANCE_SHA_RE = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/store.mjs` around lines 492 - 501, Update PROVENANCE_SHA_RE and the validation used by provenanceLines to accept both 40-character SHA-1 and 64-character SHA-256 hexadecimal object IDs, preserving the existing commit: and base: provenance output and provenanceBytes consistency.packages/harness/lib/knowledge/apply.mjs (2)
683-694: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider binding the digest over the envelope, not only
parsed.ops.Line 683 hashes
JSON.stringify(parsed.ops)alone.promotion.branchKeysits outside that binding, so a hand-edited op-set can repoint the source bucket while the digest still validates. The per-opsrc.sha256check at Line 1036 limits the damage — a swapped bucket only applies when it holds a byte-identical source learning — so this is hardening, not an exploit path.If you widen the binding,
promotionDigestinpromote.mjsmust hash the same shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/apply.mjs` around lines 683 - 694, Bind the promotion digest to the complete relevant envelope, including promotion.branchKey, instead of hashing parsed.ops alone. Update the digest verification near the apply flow and the promotionDigest implementation in promote.mjs to serialize and hash the identical shape, preserving rejection on mismatches.
721-737: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive the git context once and reuse it for both provenance and routing.
Line 727 calls
deriveGitContext, and Line 736 callsresolveWriteLayer, which callsderiveGitContextagain internally (layer.mjsLine 28). This spawns the git plumbing twice per apply run. It also lets the two reads disagree: if HEAD moves between them, the stampedwriteProvenancenames one commit while the layer routes from another.
resolveWriteLayeralready returnscontext. Resolve routing first, then buildwriteProvenancefromrouting.context.♻️ Proposed refactor
- const gitContext = deriveGitContext({ workspace, home }); - const writeProvenance = { commit: gitContext.headSha, branch: gitContext.branch, base: gitContext.baseSha }; - - // Write-layer routing (blueprint P4): ... - const routing = resolveWriteLayer({ workspace, home, layerOverride: layer === 'golden' ? 'golden' : null, log }); + // Write-layer routing (blueprint P4): ... + const routing = resolveWriteLayer({ workspace, home, layerOverride: layer === 'golden' ? 'golden' : null, log }); + // One HEAD read backs BOTH the stamp and the routing decision, so a claim's + // recorded commit can never name a different HEAD than the layer it landed in. + const writeProvenance = { + commit: routing.context.headSha, + branch: routing.context.branch, + base: routing.context.baseSha, + };Drop the now-unused
deriveGitContextimport if nothing else in this module uses it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/apply.mjs` around lines 721 - 737, Update the apply flow around resolveWriteLayer to resolve routing first, then build writeProvenance from routing.context so provenance and layer selection share one git-context read. Remove the direct deriveGitContext call and its import from this module if no other references remain, while preserving the existing routing options and provenance fields.packages/harness/lib/knowledge/layer.mjs (1)
151-162: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winA failed meta rewrite leaves the bucket moved with a stale
meta.branch.
fs.renameSyncruns first. If the followingfs.writeFileSyncthrows, thecatchreturnsnull, so the caller records no migration. The directory has already moved to the new key, and itsmeta.jsonstill names the old branch.That state is self-inconsistent for the readers of
meta.branch: doctor K6 reports a misroute, doctor K5 evaluates the wrong branch, andbucketAncestryOkstill reads the old base. Write the updatedmeta.jsoninto the source directory before the rename, or restore the original name when the rewrite fails.♻️ Proposed ordering change
try { - fs.renameSync(source.dir, target); - const meta = readBucketMeta(target) || {}; - fs.writeFileSync( - path.join(target, 'meta.json'), - JSON.stringify({ ...meta, branch: context.branch, branchKey: context.branchKey }) + '\n', - 'utf8' - ); + const meta = readBucketMeta(source.dir) || {}; + fs.writeFileSync( + path.join(source.dir, 'meta.json'), + JSON.stringify({ ...meta, branch: context.branch, branchKey: context.branchKey }) + '\n', + 'utf8' + ); + fs.renameSync(source.dir, target); return { migrated: true, from: source.key, to: context.branchKey }; } catch { return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/layer.mjs` around lines 151 - 162, Update the migration flow around the rename and metadata write so the new branch and branchKey metadata is persisted before moving the directory, or restore the original directory name if that write fails. Ensure a failed migration cannot leave the bucket at the target path with stale meta.branch data, while preserving the existing success result and null failure behavior.packages/harness/lib/doctor.mjs (1)
425-433: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winK5 spawns
git show-refonce per bucket.
branchExistsinpackages/harness/lib/knowledge/layer.mjscallslistRefs, which runsgit show-refon every invocation with no memoization. This loop calls it once per bucket. With many buckets,harness doctorpays N process spawns, each carrying a 10 s timeout.The ref list does not change during the check. Read it once and test each bucket branch against that list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/doctor.mjs` around lines 425 - 433, Update the orphan-check loop in the doctor flow to read the workspace’s Git refs once before iterating buckets, then test each bucket’s branch against that cached ref list instead of calling branchExists per bucket. Reuse the existing ref-list and branch-matching helpers from knowledge/layer.mjs where applicable, while preserving the current orphan detection behavior.packages/harness/lib/knowledge/consolidate.mjs (1)
247-259: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
bucketKeybefore building the bucket path.
layerViewselects the branch layer fromrouting?.layer === 'branch'alone. IfresolveWriteLayerever returnslayer: 'branch'with a nullbucketKey,bucketDirFor(dir, null)callspath.joinwith a null segment and throws aTypeErrorinsideconsolidateStatus.consolidateStatusis a read surface called fromrunOrientanddoctor.mjs, so a throw there is not local. The write path inpackages/harness/lib/knowledge/apply.mjsalready pairs both conditions (routing.layer === 'branch' && routing.bucketKey). Mirror that pairing here.🛡️ Proposed defensive fix
- const layer = routing?.layer === 'branch' ? 'branch' : 'golden'; - const bucketKey = layer === 'branch' ? routing.bucketKey : null; + const layer = routing?.layer === 'branch' && routing.bucketKey ? 'branch' : 'golden'; + const bucketKey = layer === 'branch' ? routing.bucketKey : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/lib/knowledge/consolidate.mjs` around lines 247 - 259, Update layerView so it selects the branch layer only when routing.layer is 'branch' and routing.bucketKey is present, mirroring the paired condition used by the write path. Ensure bucketDirFor is never called with a null bucketKey, while preserving the golden-layer fallback when either condition is absent.packages/harness/test/store-migration.test.mjs (1)
58-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail loudly instead of guessing
'main'.The docblock states that the pin exists so writes route GOLDEN. If
symbolic-ref --short HEADreturns nothing, line 59 pins the literal'main'. When the workspace branch is notmain, the pinned default no longer matches, layer routing falls back to branch-local, and the four call sites lose the precondition they were added to establish — without any test failure.Assert that the branch resolved.
♻️ Proposed refactor
function pinDefaultBranch(c) { - const branch = git(c.ws, ['symbolic-ref', '--short', 'HEAD']).stdout.trim() || 'main'; + const branch = git(c.ws, ['symbolic-ref', '--short', 'HEAD']).stdout.trim(); + assert.ok(branch, 'fixture workspace must have a resolvable branch for the GOLDEN-routing pin'); const { dir } = ensureStore(c.ws, { home: c.harnessHome }); fs.writeFileSync(path.join(dir, 'config.json'), JSON.stringify({ mode: 'on', commit: 'none', defaultBranch: branch }) + '\n'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/store-migration.test.mjs` around lines 58 - 62, Update pinDefaultBranch to fail when symbolic-ref --short HEAD resolves to an empty value instead of defaulting to 'main'; assert that branch is present before writing config.json, while preserving the existing resolved branch in defaultBranch.packages/harness/test/doctor-structural.test.mjs (1)
58-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegister temp-directory cleanup with
t.after.Each test calls
fs.rmSyncat the end of the test body. If an assertion throws, the workspace and home directories stay inos.tmpdir().withHomealready usest.afterin this file. Use the same mechanism for the directories.♻️ Proposed refactor
-function withHome(t, home) { +function withHome(t, home) { const saved = process.env.HARNESS_HOME; process.env.HARNESS_HOME = home; t.after(() => { if (saved === undefined) delete process.env.HARNESS_HOME; else process.env.HARNESS_HOME = saved; }); } + +function tempTree(t, prefix) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + return dir; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/doctor-structural.test.mjs` around lines 58 - 70, Update the test using structuralChecks to register cleanup for ws and home with t.after immediately after creating them, matching the existing withHome cleanup pattern. Remove the direct fs.rmSync calls from the test body so cleanup runs even when an assertion fails.packages/harness/test/structural-index.test.mjs (2)
88-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote the two different
readStructuralIndexshapes.This suite imports
readStructuralIndexfromlib/repo-map/structural-index.mjsand readsindex.symbolsas an object keyed by symbol name (line 92).packages/harness/test/structural-shape-compat.test.mjsline 53 andpackages/harness/test/structural-expectations.test.mjsline 321 import a function with the identical name fromlib/structural/shape.mjsand readindex.symbolsas an array of rows.Both are correct today, and
structural-shape-compat.test.mjsexists to bridge them. The identical export name across two modules with divergent return shapes is still easy to misread. Consider aliasing the import in one of the suites, for examplereadCompactIndex, to make the distinction visible at the call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/structural-index.test.mjs` around lines 88 - 92, Alias the readStructuralIndex import from lib/repo-map/structural-index.mjs in structural-index.test.mjs to a name such as readCompactIndex, and update its call sites accordingly. Leave the separate lib/structural/shape.mjs import and its array-shaped contract unchanged.
331-336: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity Misconfiguration (CWE-693)
Reachability: Internal · Exploitability: Theoretical
Reachability path
● Entry packages/harness/test/index-structural-cli.test.mjs │ ▼ ● Hop packages/harness/lib/repo-map/structural-index.mjs:47 git │ ▼ ● Hop packages/harness/test/doctor-structural.test.mjs │ ▼ ● Hop packages/harness/lib/doctor.mjs:537 head │ ▼ ● Hop packages/harness/test/provenance.test.mjs │ ▼ ● Hop packages/harness/lib/knowledge/apply.mjs:816 recordContentFailure: Promotion rejections NEVER record quarantine strikes (blueprint §5): │ ▼ ● Sink packages/harness/test/structural-index.test.mjsTighten the network-guard pattern and document its scope.
Match both quote styles for
node:httpandnode:https. This source-text scan checks only six files, matches comments and strings, and does not follow transitive imports. It does not prove that the read path is network-free.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/structural-index.test.mjs` around lines 331 - 336, Update the network-guard regex in the structural-index test to match both single- and double-quoted node:http and node:https imports, and document that the scan covers only six files, includes comments and strings, and does not inspect transitive imports; do not describe it as proving the entire read path is network-free.packages/harness/test/provenance.test.mjs (2)
208-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the episode-block size instead of the magic
150.Line 214 adds a hard-coded
150as slack for the appended episode block. The constant has no derivation in the test, and the slack makes the assertion pass for a wide range of byte counts. The assertion then no longer proves that claim bytes, not provenance bytes, caused the rejection.Measure the delta directly instead: capture
bytesandprovenanceBytesbefore the STRENGTHEN, then assert that the claim-byte growth alone crossed the cap.♻️ Proposed refactor
+ const beforeStrengthen = listLearnings(dir2).find((l) => l.id === 'sql/probe'); + const claimBytesBefore = beforeStrengthen.bytes - provenanceBytes(beforeStrengthen.fm); if (strengthened.exitCode !== 0) { assert.equal(strengthened.rejected[0].code, 'E_BYTE_CAP'); const after = listLearnings(dir2).find((l) => l.id === 'sql/probe'); - assert.ok(after.bytes - provenanceBytes(after.fm) + 150 > LEARNING_BYTE_CAP, 'rejection driven by claim bytes'); + const claimBytesAfter = after.bytes - provenanceBytes(after.fm); + assert.ok(claimBytesAfter >= claimBytesBefore, 'claim bytes never shrink across a strengthen'); + assert.ok(claimBytesBefore <= LEARNING_BYTE_CAP, 'the pre-strengthen claim body was under the cap'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/provenance.test.mjs` around lines 208 - 215, Replace the hard-coded 150 in the strengthened rejection assertion with a measured episode-block delta: capture the learning’s bytes and provenanceBytes before the STRENGTHEN, then compare the post-strengthening claim-byte total against the cap using only the observed claim-byte growth. Keep the E_BYTE_CAP check and ensure the assertion demonstrates rejection from claim bytes rather than provenance bytes.
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp directories leak when an assertion throws. Both suites create workspaces and homes with
fs.mkdtempSyncand remove them with trailing statements in the test body, or not at all. A failing assertion skips the removal, so every failed run leaves Git repositories and knowledge stores inos.tmpdir(). The shared fix is to bind removal to the test context witht.after, whichpackages/harness/test/structural-index.test.mjslines 282-287 already does correctly.
packages/harness/test/provenance.test.mjs#L23-L23: extend thetempDirhelper to accept the test context and registert.after(() => fs.rmSync(dir, { recursive: true, force: true })), then thread the context throughgitWorkspaceand everytempDircall site. This suite currently removes nothing at all.packages/harness/test/doctor-structural.test.mjs#L58-L70: replace the trailingfs.rmSyncpairs in each of the five tests with at.after-registered helper, matching the patternwithHomealready uses in the same file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/provenance.test.mjs` at line 23, The temporary-directory cleanup must run even when tests fail. In packages/harness/test/provenance.test.mjs at lines 23-23, update tempDir to accept the test context and register t.after cleanup, then pass that context through gitWorkspace and every tempDir call site. In packages/harness/test/doctor-structural.test.mjs at lines 58-70, replace each of the five tests’ trailing fs.rmSync cleanup pairs with t.after-registered cleanup matching the existing withHome pattern.packages/harness/test/structural-expectations.test.mjs (1)
543-571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the enforcement-mode and per-check-severity precedence.
These two tests pin per-check severity under
enforcement: enforce.loadPolicyalso acceptsobserveandwarnas the global mode. The precedence when a globalobservemode meets a per-checkseverity: enforceis not covered here, and that combination is the one most likely to surprise an operator.Add a case that sets
enforcement: observewithchecks.structural-expectations.severity: enforceand pins the resulting outcome and exit code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/structural-expectations.test.mjs` around lines 543 - 571, Add a structural verification test alongside the existing severity tests using global enforcement observe and structural-expectations severity enforce, then assert the resulting outcome and enforcementExitCode to pin the precedence behavior. Reuse structuralWorkspace, writeStructuralIndex, verifyFlags, and the existing structural check assertions; keep the test focused on this observe-versus-enforce combination.packages/harness/test/layered-overlay.test.mjs (2)
145-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
path.join(bucketDir, '')alias.
path.join(bucketDir, '')normalizes tobucketDir.bucketDir2is therefore the same directory asbucketDir. The indirection suggests a second, different bucket and misleads the reader. Write directly tobucketDir.♻️ Proposed change
writeLearning(dir, 'sql/human', { trigger: 'human trigger', body: 'Human golden.', source: 'human' }); - const bucketDir2 = path.join(bucketDir, ''); - writeLearning(bucketDir2, 'sql/human', { trigger: 'human trigger', body: 'Branch challenger.' }); + writeLearning(bucketDir, 'sql/human', { trigger: 'human trigger', body: 'Branch challenger.' });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/layered-overlay.test.mjs` around lines 145 - 146, Remove the redundant bucketDir2 alias and update the writeLearning call in the layered overlay test to pass bucketDir directly, preserving the existing 'sql/human' path and payload.
79-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test title claims "no git calls" but the test does not verify that.
The title states "no layer fields, no git calls". The test asserts the ranked output only. It never asserts that no Git process is spawned. The second half also creates an empty
branches/directory.loadLayeredLearningsreturns early only whenbranches/does not exist, so with the directory present it callsderiveGitContext, which spawns Git. The zero-spawn fast path is therefore not covered.Either drop "no git calls" from the title, or add an explicit assertion for the fast path. The blueprint treats the zero-spawn property as a requirement, so an assertion is preferable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/layered-overlay.test.mjs` around lines 79 - 93, The test around rankLearnings must verify the title’s no-Git-calls guarantee. Instrument or stub the Git process-spawn path, assert zero invocations for the missing branches/ fast path, and keep the existing byte-identical output assertions; ensure the empty branches/ case is tested separately or rename the title so it does not incorrectly claim zero spawns.packages/harness/test/layer-routing.test.mjs (3)
239-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the specific error on the write path.
assert.throwswithout a predicate passes for any thrown error. IfapplyOpslater fails for an unrelated reason, for example a rejected ops path, this assertion still passes and the schema guard on the write path goes untested. Reuse the same predicate applied at lines 236-238.♻️ Proposed change
- assert.throws(() => applyOps({ workspace: ws, opsPath: writeOps(ws, [addOp(ws, { slug: 'nope' })]), home })); + assert.throws( + () => applyOps({ workspace: ws, opsPath: writeOps(ws, [addOp(ws, { slug: 'nope' })]), home }), + (err) => err.code === 'E_STORE_SCHEMA' + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/layer-routing.test.mjs` at line 239, Update the applyOps assertion in the write-path test to pass the same error predicate used by the assertion at lines 236-238, ensuring it validates the expected schema-guard error rather than any thrown error.
242-263: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd the ambiguous-rename counter-case.
The test title states that migration happens "when unambiguous". The test only covers the unambiguous path.
migrateRenamedBucketmoves bucket contents on disk. If the guard for the ambiguous case regresses, two branch buckets can merge and no test detects it. Add a case with two candidate orphan buckets and assert thatmigrateRenamedBucketreturns{ migrated: false }and leaves both bucket directories in place.Do you want me to generate that test case?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/layer-routing.test.mjs` around lines 242 - 263, Add an ambiguous rename test alongside the existing migration test, creating two candidate orphan bucket directories for the workspace before calling migrateRenamedBucket. Assert it returns { migrated: false } and verify both bucket directories remain present, ensuring ambiguous candidates are not merged or moved.
303-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a static import for
consolidate.mjs.Every other harness module in this file uses a static import at the top. The dynamic import provides no isolation, because the ES module registry caches the module for the process lifetime. It also forces the test callback to be
asyncwithout need. Move the import to the top of the file.♻️ Proposed change
import { runDoctor } from '../lib/doctor.mjs'; +import { consolidateStatus } from '../lib/knowledge/consolidate.mjs'; import { writeSession } from '../lib/session.mjs';-test('listBuckets sees a routed bucket and consolidate status reports the branch lane', async () => { +test('listBuckets sees a routed bucket and consolidate status reports the branch lane', () => {- const { consolidateStatus } = await import('../lib/knowledge/consolidate.mjs'); const status = consolidateStatus({ workspace: ws, home, copilotHome: tempDir('route-ch2-') });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/layer-routing.test.mjs` around lines 303 - 313, Replace the dynamic consolidate.mjs import inside the test with a static top-level import, following the existing import style in the file. Update the test callback to remove unnecessary async usage while preserving its consolidateStatus call and assertions.packages/harness/test/layer-maintenance.test.mjs (1)
79-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the fixture commits through the config-neutralizing
git()helper.The local
git()helper at lines 16-22 setsGIT_CONFIG_GLOBAL=/dev/nullandGIT_CONFIG_SYSTEM=/dev/null. These fixture commits callspawnSync('git', ...)directly, so they inherit the developer's global Git configuration. A globalcommit.gpgsign=true, a global hooks path, or a globalcore.autocrlfsetting changes the outcome of the commit. The commit then fails silently, because the return status is not checked, and the purge under test runs against a different tree state than intended.The same pattern repeats at lines 110-111, 134-135, and 153-154 in this file. Add an identity-carrying variant of the helper and use it at all four sites.
♻️ Proposed refactor
function git(cwd, args) { return spawnSync('git', args, { cwd, encoding: 'utf8', env: { ...process.env, GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_SYSTEM: '/dev/null' }, }); } + +/** Commit the store fixture with a fixed identity and no ambient git config. */ +function commitFixture(dir, message = 'fixture') { + git(dir, ['add', '-A']); + const res = git(dir, ['-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', '-qm', message]); + assert.equal(res.status, 0, res.stderr); +}- spawnSync('git', ['add', '-A'], { cwd: dir }); - spawnSync('git', ['-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', '-qm', 'fixture'], { cwd: dir }); + commitFixture(dir);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/layer-maintenance.test.mjs` around lines 79 - 80, Update the local git helper used by the fixture setup to provide an identity-carrying variant that preserves its config-neutralizing environment while adding the temporary user.name and user.email settings. Replace the direct add/commit spawnSync calls at all four fixture-commit sites with this helper, ensuring commit results are handled consistently with the existing helper behavior.packages/harness/test/knowledge-promote.test.mjs (1)
225-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive all chunk expectations from
MAX_OPS_PER_RUNImport
MAX_OPS_PER_RUNfromconsolidate.mjs;promote.mjsdoes not export it. Derive the seed count, count assertions, and expected ID lists from the constant so changes to the limit do not make this test stale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/knowledge-promote.test.mjs` around lines 225 - 233, Update the test case around buildPromotionOps to import MAX_OPS_PER_RUN from consolidate.mjs, then derive the seeded chunk count, emitted and remaining assertions, and expected ID lists from that constant instead of hard-coded values. Do not import the constant from promote.mjs, which does not export it.packages/harness/test/knowledge-status.test.mjs (1)
119-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass an explicit
copilotHometo exercise index status.Without
copilotHome,indexStatusthrows while resolving its path, andknowledgeStatussilently setsdrifttonull. Use a temporary directory so this test covers the read-only index probe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/harness/test/knowledge-status.test.mjs` around lines 119 - 127, Update the knowledgeStatus call in the read-only test to pass the temporary home directory explicitly as copilotHome, while preserving the existing workspace and home arguments. Ensure the test exercises indexStatus successfully and continues verifying that status does not materialize the store.
…x, and verify severity
00dcfcf to
6483f03
Compare
|
All 29 review findings are addressed in 6483f03 (1 Critical, 12 Major inline — each answered and resolved on its thread — plus the outside-diff major and the 16 minors):
Note on history: two interim commits addressing this review were replaced by the single verified commit 6483f03 (same fixes independently verified, one fix reworked to gate per-file on the recorded extractor tier rather than skipping the whole check, four missing regression tests added). Verification: full suite 794 pass / 0 fail; @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/harness/lib/structural/expectations.mjs`:
- Around line 57-63: The per-file tier gate currently treats missing
fileEntry.tier as lexical, even when index metadata declares another extractor
tier. In the expectations logic around tierSkipped and the structural index
metadata, fall back to index.meta.extractorTier when fileEntry.tier is absent,
then skip non-lexical tiers as before. Add a regression test covering an absent
file tier with meta.extractorTier set to treesitter.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d537c32-480b-4945-b627-d61f177e2ea7
📒 Files selected for processing (34)
docs/MEMORY-MODEL.mddocs/architecture/engineer-harness.mdknowledge/proposals/harness-evolution-blueprint.mdpackages/harness/lib/commands.mjspackages/harness/lib/context-pack.mjspackages/harness/lib/events.mjspackages/harness/lib/flags.mjspackages/harness/lib/knowledge/admin.mjspackages/harness/lib/knowledge/apply.mjspackages/harness/lib/knowledge/consolidate.mjspackages/harness/lib/knowledge/layer.mjspackages/harness/lib/knowledge/overlay.mjspackages/harness/lib/knowledge/promote.mjspackages/harness/lib/knowledge/prune.mjspackages/harness/lib/knowledge/store.mjspackages/harness/lib/policy.mjspackages/harness/lib/repo-map/index.mjspackages/harness/lib/repo-map/structural-index.mjspackages/harness/lib/repo-map/treesitter-extractor.mjspackages/harness/lib/report.mjspackages/harness/lib/structural/expectations.mjspackages/harness/lib/structural/shape.mjspackages/harness/test/doctor-structural.test.mjspackages/harness/test/hand-edits.test.mjspackages/harness/test/knowledge-promote.test.mjspackages/harness/test/layer-maintenance.test.mjspackages/harness/test/layer-routing.test.mjspackages/harness/test/layered-overlay.test.mjspackages/harness/test/report.test.mjspackages/harness/test/store-migration.test.mjspackages/harness/test/structural-expectations.test.mjspackages/harness/test/structural-index.test.mjspackages/harness/test/structural-shape-compat.test.mjspackages/harness/test/treesitter-extractor.test.mjs
🚧 Files skipped from review as they are similar to previous changes (25)
- docs/architecture/engineer-harness.md
- packages/harness/lib/knowledge/overlay.mjs
- packages/harness/test/doctor-structural.test.mjs
- packages/harness/lib/flags.mjs
- packages/harness/lib/report.mjs
- packages/harness/test/structural-shape-compat.test.mjs
- packages/harness/lib/context-pack.mjs
- packages/harness/lib/policy.mjs
- packages/harness/test/layer-maintenance.test.mjs
- packages/harness/lib/repo-map/index.mjs
- packages/harness/lib/knowledge/promote.mjs
- packages/harness/lib/knowledge/prune.mjs
- packages/harness/lib/events.mjs
- packages/harness/lib/knowledge/layer.mjs
- packages/harness/test/layered-overlay.test.mjs
- packages/harness/lib/commands.mjs
- packages/harness/lib/structural/shape.mjs
- packages/harness/lib/knowledge/store.mjs
- packages/harness/lib/knowledge/consolidate.mjs
- packages/harness/test/structural-expectations.test.mjs
- packages/harness/lib/repo-map/treesitter-extractor.mjs
- packages/harness/lib/knowledge/admin.mjs
- packages/harness/lib/repo-map/structural-index.mjs
- packages/harness/test/layer-routing.test.mjs
- packages/harness/lib/knowledge/apply.mjs
93db07c to
67100ef
Compare
Independent review round — 9 P1s found and fixedI ran my own three-lens adversarial review (knowledge-layer correctness, security, structural/verify) over the full branch diff, deliberately excluding the themes the earlier automated round already closed. It found nine P1-class defects, all cross-module or contract-level. Two reviewers independently converged on two of them. All are now fixed across three commits, with regression tests that fail before and pass after. Promotion lane could mint protected, primitive-eligible golden knowledge
Verification integrity
Layer boundary leaked in both directionsHand-deleting a branch-local learning wrote a store-wide Structural index
Also fixed: unredacted uncapped repo text reaching evidence twice; the wasm loader's JS entry point now hash-pinned alongside the wasm (a missing lock is a loud refusal, not a silent disable); quarantine made store-global (branch-switching no longer resets three-strikes); Deferred, documented not hidden: branch-local learnings remain unreachable from Verification: full suite 843 pass / 0 fail (from 794); |
2 participants
Summary
Delivers the complete harness evolution cycle on one branch: the approved design blueprint, the phase plans, and the implementation of all four phases — built as three parallel workstreams and integrated with one commit each.
Design (approved)
knowledge/proposals/harness-evolution-blueprint.md— nine-proposal adaptation blueprint, three-lens adversarially reviewed, Human Decision: Approved 2026-08-06 with the §4/§5/§5a normative clauses as binding conditions.docs/plans/2026-08-06-feat-harness-evolution-phase1-plan.md(live plan, nowstatus: review, all ACs ticked) +docs/plans/harness-evolution-phase-drafts.md(Phases 2–4 scoping).Implementation
feat: branch-aware two-layer knowledge store (Phases 1–2)
commit:/branch:/base:provenance stamped on episodes and learnings, preserved across every re-render; provenance excluded from the 1200-byte learning cap (no strike risk).init_repo/recall/validate_plan/indexnow record).lib/git-context.mjs: deterministic<slug>-<8hex>branch keys, detached-HEAD detection, merge-base derivation.branches/<branch-key>/with per-bucket ledgers and schema marker; write routing derived from git context at write time, failing closed to branch-local; layer-aware absorb/purge/rebuild/mirror (§5a);knowledge status/promote/prune; promotion lane with hash-verified candidacy exemption, never-strike rejections, chunked--all, and theabsorb-branchledger action whose replay rule is regression-tested (retire→absorb-branch→rebuildstays retired); doctor K5–K7; per-layer SLO attribution.feat: optional tree-sitter structural index (Phase 3)
web-tree-sitter@0.25.10+ TS/JS/Python/Java grammar WASMs as Sonatype-vetted pinned optionalDependencies; sha256grammars.lockverified on the same bytes instantiated; loud lexical fallback (doctor S1 fails); required runtime deps stay at 1.~/.harness/index/<repo-id>/structural/(files/symbols/graph/meta), incremental via mtime+size → sha256,harness index --structural [--since <ref>]with validated refs; async parsing confined to the index command — orient stays sync and prefers current structural tables (byte-identical otherwise); secrets redacted at write,inertLineat render; three-audience output (ledger / bounded JSON envelope / ≤1000-token agent digest).feat: per-check verify severity + structural-expectations (Phase 4)
checks:map (advisory/warn/enforce, v1 files byte-identical); advisory failures never flip the outcome and are preserved in evidenceadvisoryFailures.structural-expectationscheck: unplanned symbol changes vs## Impacted Files, removed exported symbols with surviving callers, optional planstructural_expectations:block; skips (never fails) on missing/stale index.chore: registry
candidateentries for the three shipped surfaces (per the approval conditions);docs/MEMORY-MODEL.mdupdated to describe the shipped layered store; tool-contract rows added additively with the events footnote corrected.Verification
origin/main(feat: report real Copilot billing and harness usage; retire the evaluation tree #38 eval-tree retirement absorbed; checks.yaml consistency confirmed). Baseline was 655 — 116 new tests from the three workstreams plus integration seam coverage.harness validate-plangreen on the Phase 1 plan;prompt-library-contracts31/31 at every integration step; asset parity green.origin/mainand MERGEABLE.Summary by CodeRabbit