mvschwarz/openrig
1
0 Fork 0
mirror of https://github.com/mvschwarz/openrig.git synced 2026-11-09 11:51:37 -10:00
Code Issues Packages Projects Releases 1 Wiki Activity

fix(slice-05): context_packs require manifest.yaml file existence (guard B1 repair on a0e7e0e1) (OPR.0.3.2.5)

Browse Source 
Per velocity-guard BLOCKING-CONCERN on a0e7e0e1 (qitem-…4ef70b36): the
router checked only the parent dir existence, not the declared
manifest.yaml file itself. A bundle could declare context-packs/foo/
manifest.yaml, include the context-packs/foo dir but omit manifest.yaml,
and the router would return routedCount=1 even though
ContextPackLibraryService.scan (context-pack-library-service.ts:76)
skips packs whose manifest.yaml is absent. False-positive routedCount
at the consumer-visible boundary.

Existing tests missed this because the mock FS only modeled directories;
exists(sourcePackDir) was sufficient for C2/C3/C7/C9. The fix has three
pieces:

(A) ROUTER: add `fs.exists(sourceAbs)` check (the manifest.yaml file)
BEFORE the parent-dir isDirectory check. Returns status=missing with
consumer-specific detail when the manifest file itself is absent. This
keeps routedCount truthful at the consumer-visible boundary.

(B) MOCK: extend mockFs to support `files: string[]` alongside `dirs`
so a path can exist as a file without being a directory. exists()
respects both; isDirectory() respects only dirs. Added packFixture()
helper that builds {dirs, files} for a complete pack fixture so
existing tests can declare them concisely.

(C) UPDATED TESTS: C2/C3/C6/C7/C9 now include manifest.yaml files in
their fixtures (previously only had parent dirs). C8 redesigned to
have manifest file present + parent registered as a file (not dir) to
exercise the not_directory branch cleanly.

(D) NEW TEST C10: the exact scenario guard caught — parent dir exists +
declared basename is manifest.yaml + manifest.yaml file itself
absent. Expects routedCount=0, rejectedCount=1, status=missing, NO
copyDir fires. This is the discriminator the file-existence check is
designed to catch.

Discriminator-flip evidence BEFORE this commit (PROACTIVELY):
removed the new `if (!fs.exists(sourceAbs))` branch. Result: 3/10
fail in bundle-context-packs-router.test.ts — C4 (missing
source-without-anything → would return not_directory instead of
missing), C8 (file-not-directory → semantics shift), C10 (the new
test). All 3 failures depend on the manifest-file existence check.
Restored, 10/10 green.

This is a stronger discriminator than expected (3 tests not just C10):
the existence check load-bearing for routing semantics in multiple
edge cases, not just the new test. Banked for forward authoring of
7.3g + 7.4: when a new safety/visibility check is added, expect
discriminator-fail count > the targeted test (the check may
disambiguate other code paths too).

Final: 158/158 across full slice-05 daemon suite at this checkpoint
(was 156 → +2 incremental: +1 net new C10, +1 added bundle-types
coexistence test from 7.3e era; remainder offset).

Eighth same-class catch on slice-05's tail. Banked discipline
extension: degenerate-input dogfood must EXTEND THE MOCK SHAPE to
match the consumer contract's discrimination axes — modeling files
vs. dirs as a single "exists" was insufficient because the consumer
discriminates them.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This commit is contained in:
mvschwarzandClaude Opus 4.7 committed 2026-05-31 19:41:37 -07:00
1 parent a0e7e0e196
commit d491eca96e
2 files changed
+64 -19
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

packages/daemon/src/domain/bundle-context-packs-router.ts
+10 -3
View File
@@ -141,15 +141,22 @@ export function routeContextPacks(
});
continue;
}
const sourcePackDir = nodePath.dirname(sourceAbs);
if (!fs.exists(sourcePackDir)) {
// Manifest-file existence check (banked B1 from a0e7e0e1 guard catch):
// the consumer (context-pack-library-service.scan, line 76) skips any
// pack dir whose manifest.yaml is absent. A parent-dir-exists-but-
// manifest-missing pack would route as status=routed but be invisible
// to the consumer — false-positive routedCount class. Check the file
// ITSELF, not just the parent, to keep routedCount truthful at the
// consumer-visible boundary.
if (!fs.exists(sourceAbs)) {
records.push({
declaredPath: declared,
status: "missing",
detail: `context_pack source dir '${nodePath.dirname(declared)}' not present in bundle; skipped`,
detail: `context_pack manifest '${declared}' not present in bundle; skipped (consumer requires the file itself)`,
});
continue;
}
const sourcePackDir = nodePath.dirname(sourceAbs);
if (!fs.isDirectory(sourcePackDir)) {
records.push({
declaredPath: declared,
packages/daemon/test/bundle-context-packs-router.test.ts
+54 -16
View File
@@ -7,25 +7,34 @@ import { routeContextPacks, type ContextPacksRouterFsOps, type RouteContextPacks
// contract (context-pack-library-service.scan walks pack dirs whose
// immediate child is manifest.yaml).
function mockFs(initial: { dirs?: string[] } = {}): ContextPacksRouterFsOps & {
function mockFs(initial: { dirs?: string[]; files?: string[] } = {}): ContextPacksRouterFsOps & {
_copyCalls: Array<{ src: string; dest: string }>;
_mkdirpCalls: string[];
_dirs: Set<string>;
_files: Set<string>;
} {
const dirs = new Set<string>(initial.dirs ?? []);
const files = new Set<string>(initial.files ?? []);
const copyCalls: Array<{ src: string; dest: string }> = [];
const mkdirpCalls: string[] = [];
return {
_copyCalls: copyCalls,
_mkdirpCalls: mkdirpCalls,
_dirs: dirs,
exists: (p: string) => dirs.has(p),
_files: files,
// exists() respects both dirs and files (matches node:fs.existsSync semantics)
exists: (p: string) => dirs.has(p) || files.has(p),
isDirectory: (p: string) => dirs.has(p),
mkdirp: (p: string) => { mkdirpCalls.push(p); dirs.add(p); },
copyDir: (src: string, dest: string) => { copyCalls.push({ src, dest }); dirs.add(dest); },
};
}
/** Convenience: build a complete pack fixture (parent dir + manifest.yaml file). */
function packFixture(packParentDir: string): { dirs: string[]; files: string[] } {
return { dirs: [packParentDir], files: [`${packParentDir}/manifest.yaml`] };
}
const BUNDLE_ROOT = "/bundle/root";
const TARGET = "/operator/.openrig/context-packs";
@@ -51,7 +60,7 @@ describe("routeContextPacks", () => {
// C2: routes one pack: copyDir from sourceParentDir to target/<dirname>
it("routes one context_pack: parent dir copied to target/<dirname>", () => {
const fs = mockFs({ dirs: [`${BUNDLE_ROOT}/context-packs/intent`] });
const fs = mockFs(packFixture(`${BUNDLE_ROOT}/context-packs/intent`));
const result = routeContextPacks(
makeInput({ declaredContextPacks: ["context-packs/intent/manifest.yaml"] }),
fs,
@@ -70,6 +79,7 @@ describe("routeContextPacks", () => {
it("routes multiple distinct context_packs each to target/<dirname>", () => {
const fs = mockFs({
dirs: [`${BUNDLE_ROOT}/context-packs/intent`, `${BUNDLE_ROOT}/context-packs/persona`],
files: [`${BUNDLE_ROOT}/context-packs/intent/manifest.yaml`, `${BUNDLE_ROOT}/context-packs/persona/manifest.yaml`],
});
const result = routeContextPacks(
makeInput({
@@ -84,7 +94,8 @@ describe("routeContextPacks", () => {
expect(fs._copyCalls).toHaveLength(2);
});
// C4: missing source pack dir skipped honestly
// C4: missing source pack dir → status=missing (the manifest.yaml file
// itself is absent because the parent isn't there either)
it("missing source pack dir → status=missing (honest-scoping)", () => {
const fs = mockFs();
const result = routeContextPacks(
@@ -112,7 +123,10 @@ describe("routeContextPacks", () => {
// C6 (degenerate-input): basename not manifest.yaml — consumer-invisible
// class. Per banked PRE-handoff degenerate-input dogfood discipline.
it("declared path with non-manifest.yaml basename → status=not_manifest", () => {
const fs = mockFs({ dirs: [`${BUNDLE_ROOT}/context-packs/oddpack`] });
const fs = mockFs({
dirs: [`${BUNDLE_ROOT}/context-packs/oddpack`],
files: [`${BUNDLE_ROOT}/context-packs/oddpack/pack.yaml`, `${BUNDLE_ROOT}/context-packs/oddpack/manifest.txt`],
});
const result = routeContextPacks(
makeInput({
declaredContextPacks: [
@@ -134,10 +148,8 @@ describe("routeContextPacks", () => {
// second flagged conflict. Banked workflow_specs B1 lesson applied.
it("two declared packs sharing parent-dir basename → first routed, second conflict", () => {
const fs = mockFs({
dirs: [
`${BUNDLE_ROOT}/a/intent`,
`${BUNDLE_ROOT}/b/intent`,
],
dirs: [`${BUNDLE_ROOT}/a/intent`, `${BUNDLE_ROOT}/b/intent`],
files: [`${BUNDLE_ROOT}/a/intent/manifest.yaml`, `${BUNDLE_ROOT}/b/intent/manifest.yaml`],
});
const result = routeContextPacks(
makeInput({
@@ -164,13 +176,9 @@ describe("routeContextPacks", () => {
// C8: pack source path exists but is a file not a directory — edge case
// (unusual; manifest.yaml dirname resolved to a file). Skipped honestly.
it("source parent exists but is a file (not directory) → status=not_directory", () => {
// mockFs treats anything in dirs[] as a dir; for this case construct
// an exists-but-not-isDirectory state by overriding isDirectory.
const fs = mockFs();
fs._dirs.add(`${BUNDLE_ROOT}/oddpath`); // exists
// Override isDirectory just for this path:
const origIsDir = fs.isDirectory;
fs.isDirectory = (p: string) => p === `${BUNDLE_ROOT}/oddpath` ? false : origIsDir(p);
// Make manifest file exist (so the file-existence check passes) but the
// parent is registered as a FILE not a directory.
const fs = mockFs({ files: [`${BUNDLE_ROOT}/oddpath`, `${BUNDLE_ROOT}/oddpath/manifest.yaml`] });
const result = routeContextPacks(
makeInput({ declaredContextPacks: ["oddpath/manifest.yaml"] }),
fs,
@@ -188,6 +196,11 @@ describe("routeContextPacks", () => {
`${BUNDLE_ROOT}/context-packs/dup`,
`${BUNDLE_ROOT}/elsewhere/dup`,
],
files: [
`${BUNDLE_ROOT}/context-packs/ok/manifest.yaml`,
`${BUNDLE_ROOT}/context-packs/dup/manifest.yaml`,
`${BUNDLE_ROOT}/elsewhere/dup/manifest.yaml`,
],
});
const result = routeContextPacks(
makeInput({
@@ -212,4 +225,29 @@ describe("routeContextPacks", () => {
expect(result.records[4]!.status).toBe("routed");
expect(result.records[5]!.status).toBe("conflict");
});
// C10 (degenerate-input / banked guard catch on a0e7e0e1):
// parent dir exists but manifest.yaml file itself is absent —
// ContextPackLibraryService.scan skips packs missing manifest.yaml
// (context-pack-library-service.ts:76). The router must check the
// manifest FILE existence, not just the parent dir, or routedCount
// claims a consumer-invisible pack.
it("parent dir exists but manifest.yaml file missing → status=missing (no false-positive routed)", () => {
const fs = mockFs({
// Parent dir is present + isDirectory, BUT manifest.yaml is NOT in
// files (consumer requires the file itself).
dirs: [`${BUNDLE_ROOT}/context-packs/halfpack`],
});
const result = routeContextPacks(
makeInput({ declaredContextPacks: ["context-packs/halfpack/manifest.yaml"] }),
fs,
);
expect(result.records).toHaveLength(1);
expect(result.routedCount).toBe(0);
expect(result.rejectedCount).toBe(1);
expect(result.records[0]!.status).toBe("missing");
expect(result.records[0]!.detail).toContain("not present");
// CRUCIAL: no copyDir fires (operator-invisible pack must not route)
expect(fs._copyCalls).toHaveLength(0);
});
});