From cebe298a0276803a3703ed6a8a50f1d455fb6492 Mon Sep 17 00:00:00 2001 From: dch0202 Date: Fri, 14 Aug 2026 10:25:30 +0900 Subject: [PATCH 1/6] feat(wiki): add diff as a second routing input (#81) Give the wiki's routing protocol a review-time entry point: AGENTS.md's routing protocol gains step 7, a diff-signal decision table plus a page-set comparison directive, and both AGENTS.md and INDEX.md's preambles now name task and diff as the protocol's two inputs. Steps 1-6 and the Hard rule paragraph are untouched. Adds tests/review-routing.bats with a negative control proving the new checks can fail. --- AGENTS.md | 23 +++++++++++- INDEX.md | 6 ++- tests/review-routing.bats | 77 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 3 deletions(-) create mode 100644 tests/review-routing.bats diff --git a/AGENTS.md b/AGENTS.md index 0b48dbb..324a64e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -32,7 +32,10 @@ skills/ingest|query|lint/ # the three operations ## Routing protocol (how to consume) -When working on a task and you need guidance from this wiki: +Routing takes one of two inputs. **Planning** routes from the *intent* — the task +you are about to do. **Review** routes from the *diff* — what the code in front of +you actually does. Steps 1-6 are the same for both; the review entry then adds +step 7. 1. Read `INDEX.md`. Match your task to a domain by its "route here when" line. - **Several domains match**: route to the domain that owns the artifact you will @@ -61,6 +64,24 @@ When working on a task and you need guidance from this wiki: row** (rows are ordered general → specific); when a general row and a precondition-bearing row both fit, take the one that preserves the stated invariant. +7. **Review entry — route from the diff, then compare the page sets.** When your + input is a change rather than a task, derive the match from what the diff does, + not from what its plan said it would do: + + | Signal in the diff | Route on | + |--------------------|----------| + | A new or changed CLI flag, subcommand, SDK call, or dependency version | the owning toolchain/platform domain — resolve it against the version present where the code runs | + | Two or more writes with no transaction around them | the owning data or storage domain — establish what a concurrent reader sees between them | + | A new lock, queue, pool, background job, or shared file | the concurrency category of the owning domain | + | A new parameter reaching a query, path, template, or permission check | security, trust-boundary category | + | A changed schema, index, or migration | databases | + | A new or changed test file, assertion, or fixture | testing | + + Then compare the page set you reached against the page set the plan named. The + pages you reached that the plan never named are the change's unplanned risk + surface; report that list as a review finding in its own right. When the two + sets match, record "no unplanned pages reached" — a stated null result and an + omitted one read the same to the next reviewer, so state it. Hard rule: never load a whole domain "for background". The index lines exist so you can decide relevance without opening pages. diff --git a/INDEX.md b/INDEX.md index 9a24186..3803c73 100644 --- a/INDEX.md +++ b/INDEX.md @@ -1,7 +1,9 @@ # Root Index — Domain Map -Route by matching your current task to a "route here when" line, then open that -domain's `index.md`. Load nothing else at this level. +Route by matching a "route here when" line, then open that domain's `index.md`. +Load nothing else at this level. What you match is your current **task** when +planning, and the **diff in front of you** when reviewing — one table, two inputs +(routing protocol step 7, `AGENTS.md`). `scaffold` domains have **no pages yet** — do not route into them expecting answers; follow the cross-pointers in their index or take the next matching seeded domain diff --git a/tests/review-routing.bats b/tests/review-routing.bats new file mode 100644 index 0000000..7df9ae4 --- /dev/null +++ b/tests/review-routing.bats @@ -0,0 +1,77 @@ +#!/usr/bin/env bats +# Tests for the review-time routing protocol: AGENTS.md step 7 (diff-signal +# decision table + page-set comparison directive) and the two-input preamble +# it and INDEX.md share. +# +# A check that has never been shown to fail is not evidence (see +# wiki/testing/quality/tests-that-cannot-fail.md). The negative-control case +# below strips step 7 from a copy of AGENTS.md and asserts the same check +# that passes on the real file fails against the stripped copy. + +setup() { + REPO_ROOT="${BATS_TEST_DIRNAME}/.." + AGENTS="${REPO_ROOT}/AGENTS.md" + INDEX="${REPO_ROOT}/INDEX.md" + CHECKER="${REPO_ROOT}/scripts/wiki-lint-prohibitions.js" +} + +step7_line_count() { + # count '7. ' lines inside the routing protocol section of a given AGENTS.md + grep -c '^7\. \*\*Review entry' "$1" +} + +step7_table_rows() { + # data rows of the step-7 table: lines starting with '|' (after leading + # whitespace) that are not the header or the separator row + awk ' + /^7\. \*\*Review entry/ { f=1 } + f && /^[[:space:]]*\| Signal in the diff/ { h=1; next } + f && h && /^[[:space:]]*\|-/ { next } + f && h && /^[[:space:]]*\|/ { print } + f && h && /^[[:space:]]*$/ { exit } + ' "$1" | wc -l | tr -d ' ' +} + +# --- normal: AGENTS.md carries step 7 with a 6-row decision table ----------- + +@test "AGENTS.md: step 7 exists in the routing protocol with a 6-row table" { + [ "$(step7_line_count "$AGENTS")" -eq 1 ] + [ "$(step7_table_rows "$AGENTS")" -eq 6 ] +} + +# --- normal: INDEX.md preamble names both inputs and cites AGENTS.md -------- + +@test "INDEX.md: preamble mentions both task and diff, and cites AGENTS.md" { + preamble="$(sed -n '1,6p' "$INDEX")" + [[ "$preamble" == *"task"* ]] + [[ "$preamble" == *"diff"* ]] + [[ "$preamble" == *"AGENTS.md"* ]] +} + +# --- error: the prohibition checker still passes on the edited file --------- + +@test "AGENTS.md: wiki-lint-prohibitions exits 0 with 0 violations" { + run node "$CHECKER" "$AGENTS" + [ "$status" -eq 0 ] + [[ "$output" == *"violations: 0"* ]] +} + +# --- boundary: the empty page-set difference is stated, not omitted (D5) ---- + +@test "AGENTS.md: step 7 states the empty-diff-result case explicitly" { + run grep -F "no unplanned pages reached" "$AGENTS" + [ "$status" -eq 0 ] +} + +# --- negative control: proves the step-7 check above can fail --------------- + +@test "negative control: a copy of AGENTS.md missing step 7 fails the step-7 check" { + stripped="${BATS_TEST_TMPDIR}/AGENTS-no-step7.md" + # drop the step-7 block: from its '7. **Review entry' line up to (but not + # including) the 'Hard rule:' paragraph that follows it. + awk '/^7\. \*\*Review entry/{skip=1} /^Hard rule:/{skip=0} !skip{print}' "$AGENTS" > "$stripped" + + [ "$(step7_line_count "$stripped")" -eq 0 ] + run grep -F "no unplanned pages reached" "$stripped" + [ "$status" -ne 0 ] +} From 1f1b85375409487fb099a29fd5bb2ce7a7f88ef0 Mon Sep 17 00:00:00 2001 From: dch0202 Date: Fri, 14 Aug 2026 10:25:33 +0900 Subject: [PATCH 2/6] docs(wiki): add flag-availability and multi-object-write-ordering pages Two review-relevant failure classes had no reachable wiki page: a CLI flag/subcommand/API method that exists locally but not at the execution site, and two objects written non-atomically with a reader that can observe the gap. Adds both pages under platforms/toolchains and backend/common/storage with sourced Do-this tables and edge cases, plus one review-voice index row each. Closes #85 --- .../storage/multi-object-write-ordering.md | 57 ++++++++++++++++++ wiki/backend/index.md | 1 + wiki/platforms/index.md | 1 + ...flag-availability-at-the-execution-site.md | 59 +++++++++++++++++++ 4 files changed, 118 insertions(+) create mode 100644 wiki/backend/common/storage/multi-object-write-ordering.md create mode 100644 wiki/platforms/toolchains/flag-availability-at-the-execution-site.md diff --git a/wiki/backend/common/storage/multi-object-write-ordering.md b/wiki/backend/common/storage/multi-object-write-ordering.md new file mode 100644 index 0000000..bb97216 --- /dev/null +++ b/wiki/backend/common/storage/multi-object-write-ordering.md @@ -0,0 +1,57 @@ +--- +id: backend-common-storage-multi-object-write-ordering +domain: backend +category: storage +applies_to: [general, aws-s3] +confidence: verified +sources: + - https://docs.aws.amazon.com/AmazonS3/latest/userguide/Welcome.html + - https://man7.org/linux/man-pages/man2/rename.2.html +last_verified: 2026-08-14 +related: [backend-common-storage-object-key-persistence, backend-common-jobs-idempotent-handlers, backend-common-concurrency-distributed-locks] +--- + +# Writing Two or More Objects With No Transaction Around Them + +## When this applies + +A change writes two or more related objects (files, blobs, S3 keys) with no +transaction wrapping the writes — a payload plus its checksum/manifest, a data file +plus an index entry, a new version plus the pointer that marks it current. Also when +reviewing such a diff: ask what a concurrent reader sees between the two writes, and +what a crash between them leaves behind. + +## Do this + +Object storage gives per-key atomicity only: S3 offers "strong read-after-write +consistency for PUT and DELETE requests," but "updates are key-based... there is no +way to make atomic updates across keys" (AWS S3 docs) — every row below is a way to +make a reader's observable state single-key despite that. + +| Case | Reader outcome it guarantees | +|------|-------------------------------| +| Write-then-publish via a pointer flipped last | Write both objects under new/unpublished names first, then atomically flip a single pointer (a `rename(2)` on the filesystem, or a single-key overwrite PUT) to the version that's complete. `rename()` "will be atomically replaced, so that there is no point at which another process... will find it missing" (POSIX/Linux `rename(2)`) — readers see either the fully-old or fully-new pair, never a mix | +| Writing the dependent object first | Write the object nothing else references yet (the checksum, the index entry) before the object readers actually discover (the payload, the listing). A reader that finds the referencing object can assume its dependency already exists; one that finds only the dependency has evidence of an in-progress write, not corruption | +| A single object carrying both payload and checksum | Fold the pair into one PUT (checksum in metadata/header, or in the same body) so there is only one key and S3's per-key read-after-write consistency is the whole guarantee — no ordering decision left to get wrong | +| Serialising the writers | When neither object can safely be ordered relative to the other (both are independently discoverable, both are mutated in place), make writers mutually exclusive instead of ordering their writes — [backend-common-concurrency-distributed-locks] owns the owner-token/TTL/fencing mechanics | + +## Edge cases + +| Case | Then | +|------|------| +| Crash in the gap between the two writes | With write-then-publish, a crash before the pointer flip leaves only unpublished/orphaned objects — no reader ever observed them, so the fix is a cleanup sweep for orphans, not a data-consistency incident. Without a pointer scheme, a crash between the two writes leaves a **permanent** half-state; nothing re-runs it unless something else does (see retries, below) | +| A fail-closed reader vs a fail-open reader hits the half-state | Fail-closed (verify the checksum/companion object exists before trusting the payload) turns a partial write into a visible outage for every request in the gap; fail-open (serve the payload regardless) turns it into silently unverified/corrupt data reaching a caller. Choose per how expensive each direction is — and state the choice in the page/runbook, since the two failure modes are opposite in cost | +| A retry re-runs the pair | Re-running the same two writes must be idempotent: writing the dependent object first is naturally repeatable, but a retry after a *partial* prior attempt (dependent object written, payload write crashed) must still converge — dedupe by a deterministic key derived from the job/request, the same shape as queue-consumer idempotency ([backend-common-jobs-idempotent-handlers]) | + +## Instead of + +| If you are about to | Do this instead | Why | +|---------------------|-----------------|-----| +| Write the payload, then write its checksum as a second, independent step | Write the checksum (or the dependent object) first, or flip a single pointer last | A reader racing the two writes should never be able to see the referencing object before its dependency exists | +| Rely on "the writes usually land together" | Pick one row above and name the reader guarantee it gives | "Usually" is exactly the gap a concurrent reader or a crash finds | +| Reach for a distributed lock as the default fix | Reach for ordering (pointer-flip, dependent-first, single-object) first; lock only when neither object can be safely ordered | Ordering has no lock-store dependency and no TTL/fencing to get wrong; a lock is the fallback, not the default | + +## Sources + +- https://docs.aws.amazon.com/AmazonS3/latest/userguide/Welcome.html#ConsistencyModel — "Amazon S3 provides strong read-after-write consistency for PUT and DELETE requests of objects... in all AWS Regions"; and "Updates are key-based. There is no way to make atomic updates across keys" — the per-key guarantee this page's patterns build on, and the exact gap they close +- https://man7.org/linux/man-pages/man2/rename.2.html — "If newpath already exists, it will be atomically replaced, so that there is no point at which another process attempting to access newpath will find it missing" — the atomic-pointer-flip mechanism for the filesystem variant diff --git a/wiki/backend/index.md b/wiki/backend/index.md index b84910d..5689f57 100644 --- a/wiki/backend/index.md +++ b/wiki/backend/index.md @@ -97,4 +97,5 @@ Match your situation to a "load when" line; load only matching pages. | Page | Load when | |------|-----------| +| [multi-object-write-ordering](common/storage/multi-object-write-ordering.md) | A diff writes two or more related objects (payload + checksum, data file + index entry, new version + the pointer that marks it current) with no transaction around the writes; reviewing such a diff for what a concurrent reader observes between the writes, or what a crash between them leaves behind | | [object-key-persistence](common/storage/object-key-persistence.md) | Persisting the result of an object-storage upload (`s3.upload()`, `lib-storage` `Upload`, a transfer manager) — choosing which response field goes in the DB column; building the read/signing path from a stored reference; migrating a column that holds URLs to keys; only large uploads 404 on read | diff --git a/wiki/platforms/index.md b/wiki/platforms/index.md index 8081da8..010c86f 100644 --- a/wiki/platforms/index.md +++ b/wiki/platforms/index.md @@ -60,6 +60,7 @@ Match your situation to a "load when" line; load only matching pages. | Page | Load when | |------|-----------| | [compiler-sysroot-on-macos](toolchains/compiler-sysroot-on-macos.md) | On macOS a non-Xcode compiler (Homebrew/MacPorts LLVM) fails with `'stdio.h' file not found`, `ld: library 'System' not found`, or a `-Wmissing-sysroot` warning naming an SDK directory that does not exist; a build works under `/usr/bin/clang` but not under the toolchain the project requires; choosing between `-isysroot`, `SDKROOT`, `CPATH`, and `LIBRARY_PATH`; separating a toolchain precondition from a code regression when only the compiled tests fail | +| [flag-availability-at-the-execution-site](toolchains/flag-availability-at-the-execution-site.md) | A diff adds a CLI flag, a new subcommand, or calls a new SDK/API method against a pinned dependency; reviewing such a diff and the tool/dependency version installed in CI, on a teammate's machine, or in the deploy image is not stated in the PR; a flag that worked on the author's machine fails, or silently no-ops, wherever it actually runs | | [version-management](toolchains/version-management.md) | "Works on my machine" from tool-version drift; a project needs a pinned language/tool version (.nvmrc, .python-version, .tool-versions); making CI use the same versions as local; onboarding a machine reproducibly; a script/cron/CI step can't find a version-managed binary (shims absent in non-interactive shells); deciding where lockfiles fit in reproducibility | ## Planned (unseeded categories) diff --git a/wiki/platforms/toolchains/flag-availability-at-the-execution-site.md b/wiki/platforms/toolchains/flag-availability-at-the-execution-site.md new file mode 100644 index 0000000..2027fb4 --- /dev/null +++ b/wiki/platforms/toolchains/flag-availability-at-the-execution-site.md @@ -0,0 +1,59 @@ +--- +id: platforms-toolchains-flag-availability-at-the-execution-site +domain: platforms +category: toolchains +applies_to: [general] +confidence: verified +sources: + - https://docs.aws.amazon.com/cli/latest/userguide/cliv2-migration-changes.html + - https://semver.org/ + - https://protobuf.dev/programming-guides/proto3/ +last_verified: 2026-08-14 +related: [platforms-toolchains-version-management, platforms-tools-bsd-vs-gnu-cli, backend-common-integrations-externally-owned-defaults] +--- + +# A CLI Flag, Subcommand, or API Method That Exists Locally but Not Where It Runs + +## When this applies + +A change adds a CLI flag, a new subcommand, or a call to a new SDK/API method against +a pinned dependency. Also when reviewing such a diff and the PR does not state the +tool/dependency version installed in CI, on teammates' machines, or in the deploy +image — the author's shell proves the flag exists on the author's shell only. + +## Do this + +| Case | Do | +|------|----| +| Flag added to a command that runs in CI | Resolve the flag against the CI image's own tool, not the author's shell: run ` --help` inside the same image/container the job uses, and pin the tool's version in that image/lockfile in the same change that adds the flag — one change, not two that can drift apart | +| Flag added to a command that runs on a user's/teammate's machine | State the minimum tool version in the change and add a preflight check that runs ` --version` and fails with a named-version error below the minimum, rather than letting the flag itself be the first thing that fails | +| A new subcommand | Confirm the subcommand appears in ` help`'s own command list at the execution site before merging, not just that it runs on the author's machine — an absent subcommand is the loudest failure in this class (non-zero exit, "unknown command"), so catch it at review instead of at run time | +| A new SDK/API method on a pinned dependency | Check the dependency version the lockfile that the deploy/runtime environment actually installs from resolves to, not the version under the author's local `node_modules`/`site-packages`; bump the pin in the same change that calls the new method | + +Version pins exist for exactly this: a new flag, subcommand, or method is new +*functionality*, and semantic versioning's own contract is that new backward-compatible +functionality is a MINOR bump (semver.org, clause 7) — so "pin the version alongside +the flag" and "the flag's presence" are the same fact stated two ways, and checking +one without the other lets them drift. + +## Edge cases + +| Case | Then | +|------|------| +| The flag/field exists syntactically but is a silent no-op in the older/deployed version | This is the case CLI flag parsers usually don't produce (unknown flags typically hard-error); it shows up instead in structured inputs — a new field on a protobuf-based request, an SDK constructor kwarg, a config-file key. Proto3 "preserves unknown fields... in the serialized output" (protobuf.dev) — but an older server's generated code has no accessor for a field it doesn't know, so the call returns 200 while the intended behavior never happens. Add an explicit assertion (response field present / behavior observed), not just an exit-code check | +| The tool is version-managed, so the authoring shell and the CI shell resolve different binaries | Confirm which binary each shell actually resolves — pin file vs PATH lookup can diverge silently ([platforms-toolchains-version-management]) | +| The same flag name means something different in the execution site's userland | A flag that exists at both sites is not the same guarantee as a flag that *behaves* the same at both — check the other userland's own docs, not just that the name is present ([platforms-tools-bsd-vs-gnu-cli]) | + +## Instead of + +| If you are about to | Do this instead | Why | +|---------------------|-----------------|-----| +| Trust that a flag works because it ran on your machine | Run the same command against the execution site's actual tool version (CI image, deploy image, teammate's pin) before merging | Your shell's version is one instance among several the diff must run under | +| Add the flag and assume a missing one always errors loudly | Check whether the input is structured (protobuf field, SDK kwarg, config key) where unknown values are silently preserved or dropped rather than rejected | Hard-error-on-unknown is a CLI-parser convention, not a universal one | +| Bump only the pin, or only add the flag, in separate changes | Land the version bump and the flag/method that depends on it together | A pin bump that lands later (or not at all) leaves the flag calling a version that doesn't have it | + +## Sources + +- https://docs.aws.amazon.com/cli/latest/userguide/cliv2-migration-changes.html — concrete version-gated CLI surface: the `--copy-props` parameter is new to `aws s3` commands in CLI v2 ("The AWS CLI version 2 adds the `--copy-props` parameter"); `aws ecr get-login-password` is "available in the AWS CLI version 1.17.10 and later, and the AWS CLI version 2" — the same binary name, gated by a specific version +- https://semver.org/ — clause 7: "Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced to the public API" — the versioning contract that a new flag/method is a version fact, not just a code fact +- https://protobuf.dev/programming-guides/proto3/ — "Proto3 messages preserve unknown fields and include them during parsing and in the serialized output" — unknown fields survive on the wire but are not exposed to code compiled without them, which is the silent-no-op mechanism for structured (non-CLI) inputs From 8e586e72f0d5ad530cdfcfe40eb1e2a5ed89edc9 Mon Sep 17 00:00:00 2001 From: dch0202 Date: Fri, 14 Aug 2026 10:37:13 +0900 Subject: [PATCH 3/6] feat(orchestrate): fixed four-lens Phase 4 review pass + report template Replace Phase 4's one-sentence review method with a fixed pass of four lenses (plan conformance, wiki re-route via AGENTS.md step 7, execution-environment reality, multi-object write ordering), each grounded in a named document, plus templates/review-report.md as the source for reviews/-rN.md's three-part finding format. Closes #82. --- skills/orchestrate/SKILL.md | 25 ++- skills/orchestrate/templates/review-report.md | 38 ++++ tests/orchestrate-review-pass.bats | 167 ++++++++++++++++++ 3 files changed, 227 insertions(+), 3 deletions(-) create mode 100644 skills/orchestrate/templates/review-report.md create mode 100644 tests/orchestrate-review-pass.bats diff --git a/skills/orchestrate/SKILL.md b/skills/orchestrate/SKILL.md index d8ef298..b0fa3f0 100644 --- a/skills/orchestrate/SKILL.md +++ b/skills/orchestrate/SKILL.md @@ -476,9 +476,28 @@ until the worker picks it up (**0** picked-up, **5** deadline expired). Then substrate: `task-create` the implement Task, then `scripts/orca-worker-start.sh --task --terminal ` to reuse that task's existing session, and wait with `scripts/orca-wait.sh`. Rework rounds are further Tasks on the same -`--terminal`.)* Review each -worktree diff (`git -C diff ...HEAD`); if a session's tests look weak, -**cross-call `test-quality-auditor` yourself** (self-call + orchestrator cross-call). +`--terminal`.)* + +Run the fixed four-lens pass on each worktree diff (`git -C diff +...HEAD`) — write the result to `reviews/-rN.md` from +`templates/review-report.md`: + +1. **Plan conformance** — diff vs. the plan's decision→page map and the + brief's `` / ``; a decision silently made + differently at implement time is a defect even when the code works. +2. **Wiki re-route from the diff** — run AGENTS.md routing protocol step 7 on + the diff itself; report any page reached that the plan never named. +3. **Execution-environment reality** — any new flag/subcommand/API/dependency: + confirm it exists in the version present where the code actually runs + (`wiki/platforms/toolchains/flag-availability-at-the-execution-site.md`). +4. **Multi-object write ordering** — 2+ files/objects/rows written without a + transaction; any ordering a concurrent reader could observe mid-flight + (`wiki/backend/common/storage/multi-object-write-ordering.md`). You are the + only reviewer who sees every worktree at once, so cross-task ordering + hazards are your job alone. + +Alongside the pass, if a session's tests look weak, **cross-call +`test-quality-auditor` yourself** (self-call + orchestrator cross-call). On shortfall, write `reviews/-rN.md`, inject §3 (rework), repeat. After 3 failed rounds, escalate. When a task is approved, return to step 1 of the dispatch loop — whatever dependency it released shows up in the next `ready-set.sh` round and diff --git a/skills/orchestrate/templates/review-report.md b/skills/orchestrate/templates/review-report.md new file mode 100644 index 0000000..f709f9f --- /dev/null +++ b/skills/orchestrate/templates/review-report.md @@ -0,0 +1,38 @@ +# Review — {TASK} round {N} + +**Verdict:** approve | rework + +## Per-lens results + +Every row must be filled in every round — a lens that was not run and a lens +that passed clean are different outcomes; an omitted row reads as a passed +one unless you say otherwise. + +| Lens | Result | +|---|---| +| 1. Plan conformance | `clean — ` or `findings: F1, F2` or `not run — ` | +| 2. Wiki re-route from the diff | `clean — ` or `findings: F1, F2` or `not run — ` | +| 3. Execution-environment reality | `clean — ` or `findings: F1, F2` or `not run — ` | +| 4. Multi-object write ordering | `clean — ` or `findings: F1, F2` or `not run — ` | + +## Findings + +Each finding must state a concrete failure scenario. If it cannot, it belongs +in **Non-blocking** below, not here. + +### F1 + +- **Observation** — +- **Failure scenario** — +- **Question** — <"why this way?" — never a directive telling the worker what + to do instead> + +## Non-blocking + +Observations that cannot state a concrete failure scenario — worth asking, +not worth blocking approval on. + +### N1 + +- **Observation** — +- **Question** — <"why this way?"> diff --git a/tests/orchestrate-review-pass.bats b/tests/orchestrate-review-pass.bats new file mode 100644 index 0000000..148dd7d --- /dev/null +++ b/tests/orchestrate-review-pass.bats @@ -0,0 +1,167 @@ +#!/usr/bin/env bats +# Tests for skills/orchestrate/SKILL.md Phase 4's four-lens review pass and +# skills/orchestrate/templates/review-report.md (i82-phase4-lenses, issue +# #82 / #84). +# +# A checker's own report is not evidence it works until it has been shown to +# fail on something (wiki/testing/quality/checks-that-cannot-pass.md) — each +# structural assertion below has a paired negative control that strips the +# asserted span from a copy and shows the same check fail +# (wiki/testing/quality/spec-artifact-checks.md). +# +# These tests do NOT assert the existence of i81 (AGENTS.md routing step 7) +# or i85 (the lens-3/lens-4 wiki pages) artifacts — those land on sibling +# branches, not this one; SKILL.md only needs to *cite* their paths. + +setup() { + REPO_ROOT="${BATS_TEST_DIRNAME}/.." + SKILL="${REPO_ROOT}/skills/orchestrate/SKILL.md" + TEMPLATE="${REPO_ROOT}/skills/orchestrate/templates/review-report.md" +} + +# Extracts the "## Phase 4" section: from its heading up to (not including) +# the next "## " heading. +phase4_section() { + awk '/^## Phase 4/{p=1} p && /^## / && !/^## Phase 4/{exit} p' "$1" +} + +# Concatenates the digit prefix of every numbered-bold lens line found in +# the Phase 4 section of the given file, e.g. "1234" when all four are +# present in order, "" when none are, "134" when one is missing. +lens_order() { + phase4_section "$1" | grep -oE '^[0-9]\. \*\*[^*]+\*\*' | sed -E 's/^([0-9])\..*/\1/' | tr -d '\n' +} + +# --- normal: the four lenses are present, numbered 1-4, in order ----------- + +@test "Phase 4 contains the four lenses, numbered 1-4, in order" { + [ "$(lens_order "$SKILL")" = "1234" ] + section="$(phase4_section "$SKILL")" + [[ "$section" == *"Plan conformance"* ]] + [[ "$section" == *"Wiki re-route from the diff"* ]] + [[ "$section" == *"Execution-environment reality"* ]] + [[ "$section" == *"Multi-object write ordering"* ]] +} + +@test "lens 2 cites AGENTS.md routing protocol step 7 by document and step number only" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *"AGENTS.md routing protocol step 7"* ]] +} + +@test "lens 3 and lens 4 cite their grounding wiki pages" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *"wiki/platforms/toolchains/flag-availability-at-the-execution-site.md"* ]] + [[ "$section" == *"wiki/backend/common/storage/multi-object-write-ordering.md"* ]] +} + +@test "lens 4 states the coordinator-only leverage" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *"only reviewer who sees every worktree at once"* ]] +} + +@test "Phase 4 instructs writing reviews/-rN.md from templates/review-report.md" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *'reviews/-rN.md'* ]] + [[ "$section" == *'templates/review-report.md'* ]] +} + +@test "Phase 4 retains the test-quality-auditor obligation alongside the pass, not as a fifth lens" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *'test-quality-auditor'* ]] + # it must not be numbered 5 — the issue fixes exactly four lenses + [[ "$section" != *'5. **'* ]] +} + +@test "Phase 4 retains the surrounding mechanics: diff command, 3-round cap, escalation, dispatch-loop return" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *'git -C diff'* ]] + [[ "$section" == *'...HEAD'* ]] + [[ "$section" == *'After 3'* ]] + [[ "$section" == *'escalate'* ]] + [[ "$section" == *'return to step 1 of the dispatch'* ]] + [[ "$section" == *'go to Phase 5'* ]] +} + +# --- negative control: stripping the lens list breaks the order check ------ + +@test "negative control: a SKILL.md copy with the lens lines removed fails the order check" { + stripped="${BATS_TEST_TMPDIR}/skill-no-lenses.md" + grep -v -E '^[0-9]\. \*\*(Plan conformance|Wiki re-route|Execution-environment|Multi-object)' "$SKILL" > "$stripped" + [ "$(lens_order "$stripped")" != "1234" ] +} + +# --- negative control: reordering two lenses breaks the order check -------- + +@test "negative control: a SKILL.md copy with lenses 2 and 3 swapped fails the order check" { + swapped="${BATS_TEST_TMPDIR}/skill-swapped-lenses.md" + awk ' + /^2\. \*\*Wiki re-route/ { line2 = $0; getline; rest2 = $0; got2 = 1; next } + /^3\. \*\*Execution-environment/ && got2 { + print $0; getline; print $0 + print line2; print rest2 + next + } + { print } + ' "$SKILL" > "$swapped" + [ "$(lens_order "$swapped")" != "1234" ] +} + +# --- template structure: three-part finding format + non-blocking section -- + +@test "review-report.md has the three-part finding format" { + content="$(cat "$TEMPLATE")" + [[ "$content" == *"Observation"* ]] + [[ "$content" == *"Failure scenario"* ]] + [[ "$content" == *"Question"* ]] + [[ "$content" == *"## Non-blocking"* ]] +} + +@test "review-report.md has a header naming task, round, and an approve/rework verdict" { + content="$(cat "$TEMPLATE")" + [[ "$content" == *"{TASK}"* ]] + [[ "$content" == *"{N}"* ]] + [[ "$content" == *"approve"* ]] + [[ "$content" == *"rework"* ]] +} + +# --- error/boundary: per-lens table distinguishes clean, findings, not-run - + +@test "review-report.md's per-lens table has 4 rows, each distinguishing clean/findings/not-run" { + content="$(cat "$TEMPLATE")" + clean_count="$(grep -c 'clean —' "$TEMPLATE")" + notrun_count="$(grep -c 'not run —' "$TEMPLATE")" + [ "$clean_count" -eq 4 ] + [ "$notrun_count" -eq 4 ] + [[ "$content" == *"findings: F1, F2"* ]] +} + +# --- negative control: a template copy missing the not-run option fails ---- + +@test "negative control: a review-report.md copy with 'not run' stripped fails the distinguishing check" { + stripped="${BATS_TEST_TMPDIR}/review-report-no-notrun.md" + sed 's/ or `not run — `//' "$TEMPLATE" > "$stripped" + notrun_count="$(grep -c 'not run —' "$stripped" || true)" + [ "$notrun_count" -eq 0 ] +} + +# --- negative control: a template copy without the non-blocking section ---- + +@test "negative control: a review-report.md copy without the Non-blocking section fails the structure check" { + stripped="${BATS_TEST_TMPDIR}/review-report-no-nonblocking.md" + awk '/^## Non-blocking/{exit} {print}' "$TEMPLATE" > "$stripped" + content="$(cat "$stripped")" + [[ "$content" != *"## Non-blocking"* ]] +} + +# --- boundary: no other test file was touched by this task ----------------- + +@test "no BATS file other than this one is new or modified in the working tree" { + cd "$REPO_ROOT" || return 1 + run git status --porcelain -- 'tests/*.bats' + [ "$status" -eq 0 ] + while IFS= read -r line; do + [ -z "$line" ] && continue + f="${line:3}" + [ "$f" = "tests/orchestrate-review-pass.bats" ] + done <<< "$output" +} From c9b391f94f70776cf5f37068b87b609610b15a44 Mon Sep 17 00:00:00 2001 From: dch0202 Date: Fri, 14 Aug 2026 11:49:45 +0900 Subject: [PATCH 4/6] feat(orchestrate): worker fix-or-answer obligation on rework prompts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit session-prompt.md §3/§O3 previously forced rework compliance ("Address the issues...") through an information gap: the coordinator sees the diff but not the constraint the worker hit. Give the worker the matching half of i82's Socratic finding format — per blocking finding, fix it or answer its Question with the concrete reason and leave it, recorded via an in-file `- **Answer (r{N})**` line; silence on any finding is not a valid resolution. §O3 additionally requires the worker_done --body to summarize per-finding outcomes so the coordinator sees the split without opening the review file. Non-blocking findings are excluded from the obligation. Also bumps the pre-existing pinned checksum in tests/send-prompt.bats ("the Orca prompt set is byte-identical"), which hashes the exact byte range covering §O3 and necessarily changed; scoped to the checksum literal and its precedent-style bump-comment only, per that test's own documented maintenance convention and coordinator sign-off. Closes #84. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TvxaV9oGPZmpKXh6EsLBh3 --- .../orchestrate/templates/session-prompt.md | 11 ++- tests/send-prompt.bats | 5 +- tests/session-prompt-rework.bats | 86 +++++++++++++++++++ 3 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 tests/session-prompt-rework.bats diff --git a/skills/orchestrate/templates/session-prompt.md b/skills/orchestrate/templates/session-prompt.md index 64aa5a1..9a44dfa 100644 --- a/skills/orchestrate/templates/session-prompt.md +++ b/skills/orchestrate/templates/session-prompt.md @@ -76,7 +76,7 @@ Approved. Implement .orchestration/plans/{TASK}.md via the loop-implement skill: ## (3) Rework — injected when review requests changes -Address the issues in .orchestration/reviews/{TASK}-r{N}.md via the loop-implement skill (re-run step 6.5 audit; never weaken or skip tests). Then run `STATUS_DIR={STATUS_DIR} sh {SKILL}/scripts/status-update.sh {TASK} impl_done worktree=$PWD` and wait. +Address the issues in .orchestration/reviews/{TASK}-r{N}.md via the loop-implement skill (re-run step 6.5 audit; never weaken or skip tests). Per finding: fix it, or answer its Question with the concrete reason and leave it — either way, append `- **Answer (r{N})** — fixed` or `- **Answer (r{N})** — stands: ` under that finding in the absolute file $(dirname {STATUS_DIR})/reviews/{TASK}-r{N}.md; silence on any finding is not a valid resolution. This obligation binds blocking findings only — answering Non-blocking findings is encouraged, not required. Then run `STATUS_DIR={STATUS_DIR} sh {SKILL}/scripts/status-update.sh {TASK} impl_done worktree=$PWD` and wait. ## (4) Merge-prep — injected after final approval @@ -135,10 +135,15 @@ Then END YOUR TURN. Do not commit, push, or PR — the orchestrator merges. ## (O3) Rework — `--spec` of the rework Task Address the issues in .orchestration/reviews/{TASK}-r{N}.md via the loop-implement skill -(re-run step 6.5 audit; never weaken or skip tests). Then run +(re-run step 6.5 audit; never weaken or skip tests). Per finding: fix it, or answer its +Question with the concrete reason and leave it — either way, append `- **Answer (r{N})** +— fixed` or `- **Answer (r{N})** — stands: ` under that finding in the absolute +file $(dirname {STATUS_DIR})/reviews/{TASK}-r{N}.md; silence on any finding is not a +valid resolution. This obligation binds blocking findings only — answering Non-blocking +findings is encouraged, not required. Then run `STATUS_DIR={STATUS_DIR} sh {SKILL}/scripts/status-update.sh {TASK} impl_done worktree=$PWD` and report exactly once: -`orca orchestration send --type worker_done --subject "impl_done: {TASK} r{N}" --body "" --task-id {ORCA_TASK_ID} --dispatch-id {ORCA_DISPATCH_ID} --outcome succeeded --files-modified "" --json` +`orca orchestration send --type worker_done --subject "impl_done: {TASK} r{N}" --body "" --task-id {ORCA_TASK_ID} --dispatch-id {ORCA_DISPATCH_ID} --outcome succeeded --files-modified "" --json` (a failure is `--outcome failed`, never failure encoded only in prose). Then END YOUR TURN. ## (O4) Merge-prep — `--spec` of the merge-prep Task diff --git a/tests/send-prompt.bats b/tests/send-prompt.bats index d74facb..e61c278 100644 --- a/tests/send-prompt.bats +++ b/tests/send-prompt.bats @@ -426,9 +426,12 @@ tpl_sections_single_line() { # Bumped from 3932390147/5667 when O1 became adopt-the-coordinator's-plan # instead of author-your-own: planning moved to the coordinator so it runs on # the planning model, not on whatever tier the worker is pinned to. + # Bumped from 4060540920/6077 when §O3 gained the fix-or-answer obligation: + # per finding, fix it or answer its Question and leave it, recorded via an + # Answer (r{N}) line in the review file; silence is not a valid resolution. run sh -c "sed -n '/^\*\*Orca substrate\.\*\*/,/^## Subagent usage protocol/p' '$TPL' | cksum" [ "$status" -eq 0 ] - [ "$output" = "4060540920 6077" ] + [ "$output" = "3269123258 6531" ] } @test "template: the Orca ask rule forbids deciding a timed-out question" { diff --git a/tests/session-prompt-rework.bats b/tests/session-prompt-rework.bats new file mode 100644 index 0000000..ace7826 --- /dev/null +++ b/tests/session-prompt-rework.bats @@ -0,0 +1,86 @@ +#!/usr/bin/env bats +# Tests for skills/orchestrate/templates/session-prompt.md §3/§O3 — the +# per-finding fix-or-answer obligation (issue #84). i82's review-report.md +# finding format (Observation/Failure scenario/Question, blocking vs +# `## Non-blocking`) is cited by these sections in prose only; that template +# lives on a sibling branch not present here, so no test below may assert its +# existence (wiki/testing/quality/checks-that-cannot-pass.md — never gate on +# a target absent from this branch). A stripped-copy negative control is +# included per the same page: a check never shown to fail on anything proves +# nothing. + +setup() { + TEMPLATE="${BATS_TEST_DIRNAME}/../skills/orchestrate/templates/session-prompt.md" +} + +extract_section() { + local heading="$1" file="$2" + awk -v h="$heading" ' + index($0, h) == 1 { flag=1; next } + /^## / { flag=0 } + flag { print } + ' "$file" +} + +# --- normal: §3 obliges fix-or-answer + the Answer-line convention ---------- + +@test "§3: obliges fix-or-answer per finding and the Answer-line convention" { + section="$(extract_section '## (3)' "$TEMPLATE")" + [[ "$section" == *"fix it, or answer its Question"* ]] + [[ "$section" == *"Answer (r{N})"* ]] + [[ "$section" == *"silence"* ]] +} + +# --- normal: §3 states the blocking-only scope ------------------------------- + +@test "§3: blocking-only scope is stated, Non-blocking answering not obliged" { + section="$(extract_section '## (3)' "$TEMPLATE")" + [[ "$section" == *"blocking"* ]] + [[ "$section" == *"Non-blocking"* ]] + [[ "$section" == *"encouraged"* ]] +} + +# --- normal: §O3 carries the same obligation --------------------------------- + +@test "§O3: obliges fix-or-answer per finding and the Answer-line convention" { + section="$(extract_section '## (O3)' "$TEMPLATE")" + [[ "$section" == *"fix it, or answer its Question"* ]] + [[ "$section" == *"Answer (r{N})"* ]] + [[ "$section" == *"silence"* ]] + [[ "$section" == *"blocking"* ]] + [[ "$section" == *"Non-blocking"* ]] +} + +# --- normal: §O3 obliges the per-finding worker_done --body summary --------- + +@test "§O3: worker_done --body must summarize per-finding outcomes" { + section="$(extract_section '## (O3)' "$TEMPLATE")" + [[ "$section" == *"--body"* ]] + [[ "$section" == *"fixed"* ]] + [[ "$section" == *"stands"* ]] +} + +# --- boundary: §3 stays exactly one physical line (send-keys -l safe) ------- + +@test "§3: is a single physical line (no embedded newlines, flatten-safe)" { + section="$(extract_section '## (3)' "$TEMPLATE")" + non_blank_lines="$(printf '%s\n' "$section" | grep -c '.')" + [ "$non_blank_lines" -eq 1 ] +} + +# --- negative control: a stripped §3 fails the obligation check ------------- + +@test "negative control: a stripped §3 (pre-i84 text) fails the obligation check" { + stripped="${BATS_TEST_TMPDIR}/stripped-session-prompt.md" + cat > "$stripped" <<'EOF' +## (3) Rework — injected when review requests changes + +Address the issues in .orchestration/reviews/{TASK}-r{N}.md via the loop-implement skill (re-run step 6.5 audit; never weaken or skip tests). Then run `STATUS_DIR={STATUS_DIR} sh {SKILL}/scripts/status-update.sh {TASK} impl_done worktree=$PWD` and wait. + +## (4) Merge-prep +EOF + section="$(extract_section '## (3)' "$stripped")" + [[ "$section" != *"fix it, or answer its Question"* ]] + [[ "$section" != *"Answer (r{N})"* ]] + [[ "$section" != *"silence"* ]] +} From e0432694cc95c1a431d2fe8d3e66e8e3943b4a4e Mon Sep 17 00:00:00 2001 From: dch0202 Date: Fri, 14 Aug 2026 11:55:33 +0900 Subject: [PATCH 5/6] feat(orchestrate): Phase 4 insight emission from confirmed review findings Add the near-miss capture rule to Phase 4: after a rework round's fix is confirmed by re-review, emit one Insight candidate per fixed `## Findings` item (never for `## Non-blocking`), using the frozen block format and 0-3/session cap, with a fenced worked example whose placeholder trigger/directive keep it from being self-harvested if ever quoted verbatim. Also fixes i82's scope-purity test (tests/orchestrate-review-pass.bats): anchor to the commit that introduced the file instead of live git-status (avoids false positives from a sibling task's uncommitted files on this shared branch), and skip honestly on a shallow clone where the adding commit cannot be resolved truthfully. Closes #83. --- skills/orchestrate/SKILL.md | 22 +++ tests/orchestrate-insight-emission.bats | 205 ++++++++++++++++++++++++ tests/orchestrate-review-pass.bats | 33 +++- 3 files changed, 253 insertions(+), 7 deletions(-) create mode 100644 tests/orchestrate-insight-emission.bats diff --git a/skills/orchestrate/SKILL.md b/skills/orchestrate/SKILL.md index b0fa3f0..e184455 100644 --- a/skills/orchestrate/SKILL.md +++ b/skills/orchestrate/SKILL.md @@ -503,6 +503,28 @@ failed rounds, escalate. When a task is approved, return to step 1 of the dispat loop — whatever dependency it released shows up in the next `ready-set.sh` round and the freed slot is refilled immediately. When `ready-set.sh` returns **5**, go to Phase 5. +**Insight emission.** After a rework round's fix is confirmed by re-review, +emit one ★ Insight candidate per finding that was fixed and confirmed — +**only** for findings that sat in `## Findings` (they carried a failure +scenario: a real defect). `## Non-blocking` items never emit — they lack a +failure scenario, so they are style/preference, not the near-miss lesson this +rule captures. Both conditions must hold: the finding was a `## Findings` +item AND the following re-review round confirmed the fix — a finding that is +caught but not yet fixed is not a near-miss lesson yet. Use the frozen block +format from `hooks/insight-instruction.sh` verbatim (`trigger`/`directive` +required, `why`/`evidence` expected); the 0–3-per-session cap still applies, +so if a round confirms more fixes than the remaining budget, prioritize the +highest-signal finding. Map the fields like this: + +``` +★ Insight ───────────────────────────────────── +trigger: +directive: +why: +evidence: reviews/-rN.md + the fixing commit +───────────────────────────────────────────── +``` + ## Splitting a task mid-run A worker may report that its task is much larger than the brief assumed. It diff --git a/tests/orchestrate-insight-emission.bats b/tests/orchestrate-insight-emission.bats new file mode 100644 index 0000000..9fa8a21 --- /dev/null +++ b/tests/orchestrate-insight-emission.bats @@ -0,0 +1,205 @@ +#!/usr/bin/env bats +# Tests for skills/orchestrate/SKILL.md Phase 4's insight-emission rule +# (i83-insight-emission, issue #83): after a rework round's fix is confirmed, +# emit one ★ Insight candidate per fixed `## Findings` item; `## Non-blocking` +# items never emit. +# +# A checker's own report is not evidence it works until it has been shown to +# fail on something (wiki/testing/quality/checks-that-cannot-pass.md) — each +# structural assertion below has a paired negative control that strips the +# asserted span from a copy and shows the same check fail +# (wiki/testing/quality/spec-artifact-checks.md). +# +# The harvest tests exercise hooks/harvest.js for real (per-test $HOME, same +# invocation pattern as tests/harvest.bats) rather than asserting keyword +# presence — this proves the emission rule's format is actually compatible +# with the frozen harvester, and that the worked example's own placeholder +# text is not itself harvested if a session ever quotes it verbatim. + +setup() { + REPO_ROOT="${BATS_TEST_DIRNAME}/.." + SKILL="${REPO_ROOT}/skills/orchestrate/SKILL.md" + HARVEST="${REPO_ROOT}/hooks/harvest.js" + + command -v node >/dev/null || { + echo "node is required to run the harvest end-to-end tests" + return 1 + } + + HOME="${BATS_TEST_TMPDIR}/home" + mkdir -p "$HOME/.dev-loop/queue" + WORK="${BATS_TEST_TMPDIR}/work" + mkdir -p "$WORK" +} + +# Extracts the "## Phase 4" section: from its heading up to (not including) +# the next "## " heading. Same technique as tests/orchestrate-review-pass.bats. +phase4_section() { + awk '/^## Phase 4/{p=1} p && /^## / && !/^## Phase 4/{exit} p' "$1" +} + +# JSON-encodes stdin (may contain real newlines) for embedding as a message +# content field. +_json_string() { + node -e ' + let s=""; process.stdin.on("data", d => s += d); + process.stdin.on("end", () => process.stdout.write(JSON.stringify(s))); + ' +} + +# Writes a one-line assistant-turn transcript at $1 whose content is $2. +_write_transcript() { # + local content_json + content_json="$(printf '%s' "$2" | _json_string)" + printf '{"message":{"role":"assistant","content":%s}}\n' "$content_json" > "$1" +} + +_run_harvest() { # + printf '{"cwd":"%s","session_id":"%s","transcript_path":"%s"}' "$WORK" "$1" "$2" | \ + HOME="$HOME" node "$HARVEST" +} + +_queue_lines() { # + local f="$HOME/.dev-loop/queue/$1.jsonl" + [ -f "$f" ] || { echo 0; return; } + awk 'NF { c++ } END { print c + 0 }' "$f" +} + +# --- normal: the emission rule states both firing conditions --------------- + +@test "Phase 4 states both emission firing conditions: fixed Findings item + confirmed re-review" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *"Insight emission"* ]] + [[ "$section" == *"## Findings"* ]] + [[ "$section" == *"confirmed"* ]] + [[ "$section" == *"re-review"* ]] +} + +# --- boundary: the Non-blocking negative is explicitly stated -------------- + +@test "Phase 4 explicitly states Non-blocking items never emit" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *"## Non-blocking"* ]] + [[ "$section" == *"never emit"* ]] +} + +# --- normal: frozen format + cap are both referenced ----------------------- + +@test "the emission rule references the frozen block format and the 0-3 cap" { + section="$(phase4_section "$SKILL")" + [[ "$section" == *"hooks/insight-instruction.sh"* ]] + [[ "$section" == *"cap"* ]] + [[ "$section" == *"highest-signal"* ]] +} + +# --- normal: worked example is fenced and maps all three fields ------------ + +@test "a fenced worked example maps trigger, directive, and evidence" { + section="$(phase4_section "$SKILL")" + fence_count="$(printf '%s\n' "$section" | grep -c '^```' || true)" + [ "$fence_count" -eq 2 ] + [[ "$section" == *'trigger:'* ]] + [[ "$section" == *'directive:'* ]] + [[ "$section" == *'evidence:'* ]] + [[ "$section" == *'reviews/-rN.md'* ]] +} + +# --- negative control: stripping the emission rule fails the presence check - + +@test "negative control: a SKILL.md copy with the emission rule removed fails the presence check" { + stripped="${BATS_TEST_TMPDIR}/skill-no-emission.md" + awk ' + /^\*\*Insight emission\.\*\*/ { skip=1 } + /^## Splitting a task mid-run/ { skip=0 } + !skip { print } + ' "$SKILL" > "$stripped" + section="$(phase4_section "$stripped")" + [[ "$section" != *"Insight emission"* ]] +} + +# --- negative control: stripping just the Non-blocking negative ------------ + +@test "negative control: a copy with the Non-blocking sentence removed fails the boundary check" { + stripped="${BATS_TEST_TMPDIR}/skill-no-nonblocking-negative.md" + grep -v 'never emit' "$SKILL" > "$stripped" + section="$(phase4_section "$stripped")" + [[ "$section" != *"never emit"* ]] +} + +# --- negative control: stripping one fence breaks the fenced-example check - + +@test "negative control: a copy with the closing fence removed fails the fenced-example check" { + stripped="${BATS_TEST_TMPDIR}/skill-broken-fence.md" + # drop the ``` fence immediately following the box-drawing closing + # delimiter line (`─────...`) — that is this block's own closing fence, + # not the unrelated ```json fences elsewhere in the file. + awk ' + { + if (skip_next && $0 ~ /^```$/) { skip_next=0; next } + print + if ($0 ~ /^─+$/) skip_next=1 + } + ' "$SKILL" > "$stripped" + section="$(phase4_section "$stripped")" + fence_count="$(printf '%s\n' "$section" | grep -c '^```' || true)" + [ "$fence_count" -ne 2 ] +} + +# --- harvest end-to-end: a coordinator-emitted block citing a review round -- +# --- is queued by hooks/harvest.js (per-test $HOME, like tests/harvest.bats) -- + +@test "harvest end-to-end: a coordinator-emitted block citing a review round is queued" { + body="★ Insight ───── +trigger: a diff added a new CLI flag with no toolchain version check +directive: confirm the flag exists in the deployed toolchain version before approving +why: lens 3 exists to catch exactly this and the first pass missed it +evidence: reviews/i83-insight-emission-r1.md + fixing commit a1b2c3d +─────" + transcript="${BATS_TEST_TMPDIR}/transcript-real.jsonl" + _write_transcript "$transcript" "$body" + + run _run_harvest "s1" "$transcript" + [ "$status" -eq 0 ] + [ "$(_queue_lines "s1")" -eq 1 ] + + qfile="$HOME/.dev-loop/queue/s1.jsonl" + [[ "$(cat "$qfile")" == *"toolchain version check"* ]] +} + +# --- normal: the worked example's own placeholders are not self-harvested -- +# --- if a session ever quotes the fenced section verbatim ------------------- + +@test "the fenced worked example, if quoted verbatim, is not itself harvested" { + section="$(phase4_section "$SKILL")" + example="$(printf '%s\n' "$section" | awk ' + /^```$/ { c++; if (c == 1) { f=1; next } else { exit } } + f { print } + ')" + [ -n "$example" ] + [[ "$example" == *'trigger:'* ]] + + transcript="${BATS_TEST_TMPDIR}/transcript-selfquote.jsonl" + _write_transcript "$transcript" "$example" + + run _run_harvest "s2" "$transcript" + [ "$status" -eq 0 ] + [ "$(_queue_lines "s2")" -eq 0 ] +} + +# --- boundary: changes vs branch HEAD name only files this task owns or was -- +# --- explicitly authorized to touch (skills/orchestrate/SKILL.md, this test -- +# --- file, and tests/orchestrate-review-pass.bats for review round 1's F1) -- + +@test "changes vs branch HEAD are exactly the files this task owns or was authorized to touch" { + cd "$REPO_ROOT" || return 1 + run git status --porcelain + [ "$status" -eq 0 ] + while IFS= read -r line; do + [ -z "$line" ] && continue + f="${line:3}" + case "$f" in + skills/orchestrate/SKILL.md|tests/orchestrate-insight-emission.bats|tests/orchestrate-review-pass.bats) : ;; + *) return 1 ;; + esac + done <<< "$output" +} diff --git a/tests/orchestrate-review-pass.bats b/tests/orchestrate-review-pass.bats index 148dd7d..59b96e8 100644 --- a/tests/orchestrate-review-pass.bats +++ b/tests/orchestrate-review-pass.bats @@ -155,13 +155,32 @@ lens_order() { # --- boundary: no other test file was touched by this task ----------------- -@test "no BATS file other than this one is new or modified in the working tree" { +@test "the commit that added this file touched no other BATS file" { + # A working-tree `git status` version of this check false-positives on any + # uncommitted, unrelated .bats file a later task in the same worktree adds + # (e.g. a sequential sibling task on this branch) — the tree it would + # inspect is someone else's in-progress state, not this commit's diff + # (wiki/qa/process/scope-purity-checks.md: prove purity from the change + # itself, not from ambient state). Anchor to the commit that introduced + # this file instead: that diff is permanent, so the check passes + # regardless of what else is uncommitted right now, and still fails for + # real if that commit ever touched a second .bats file. cd "$REPO_ROOT" || return 1 - run git status --porcelain -- 'tests/*.bats' + # A shallow clone (actions/checkout@v4 defaults to fetch-depth 1) has no + # ancestor history: `git log --diff-filter=A` resolves to the shallow + # boundary commit for every path, and that commit has no parent to diff + # against, so it reports EVERY tracked file as "added" — a depth-1 clone + # cannot answer "which commit added this file" truthfully. Skip rather than + # assert something the checkout cannot honestly prove; the check still runs + # for real on any full clone (local dev, release workflows). + if [ "$(git rev-parse --is-shallow-repository 2>/dev/null)" = "true" ]; then + skip "shallow clone (no full history) — cannot resolve the true adding commit, see wiki/qa/process/scope-purity-checks.md" + fi + commit="$(git log --diff-filter=A --format=%H -- tests/orchestrate-review-pass.bats | tail -1)" + [ -n "$commit" ] + run git show --stat --format= "$commit" -- 'tests/*.bats' [ "$status" -eq 0 ] - while IFS= read -r line; do - [ -z "$line" ] && continue - f="${line:3}" - [ "$f" = "tests/orchestrate-review-pass.bats" ] - done <<< "$output" + file_lines="$(printf '%s\n' "$output" | grep -c ' | ' || true)" + [ "$file_lines" -eq 1 ] + [[ "$output" == *"tests/orchestrate-review-pass.bats"* ]] } From f69ed14afa6648d38cb1508725b7ef67c213535e Mon Sep 17 00:00:00 2001 From: dch0202 Date: Fri, 14 Aug 2026 13:57:59 +0900 Subject: [PATCH 6/6] =?UTF-8?q?fix(tests):=20normalize=20whitespace=20befo?= =?UTF-8?q?re=20=C2=A7O3=20substring=20assertions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit session-prompt.md wraps the §O3 "fix it, or answer its Question" phrase across two physical lines. The literal-space glob in session-prompt-rework.bats couldn't match the resulting newline, so the suite passed locally (bats/eval artifact) but failed on ubuntu bash 5 in CI (PR #94, not ok 433). Add normalize_ws() and apply it to the two §O3 tests' extracted section before matching; the §3 single-physical-line structural test keeps raw extraction since it exists to detect embedded newlines. --- tests/session-prompt-rework.bats | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/tests/session-prompt-rework.bats b/tests/session-prompt-rework.bats index ace7826..77637c5 100644 --- a/tests/session-prompt-rework.bats +++ b/tests/session-prompt-rework.bats @@ -22,6 +22,14 @@ extract_section() { ' "$file" } +# Collapses embedded newlines (and the runs of whitespace they leave behind) +# to a single space, so a substring assertion survives prose being +# hard-wrapped across physical lines. Used only for substring checks, never +# for the single-physical-line structural check (that one needs raw output). +normalize_ws() { + printf '%s' "$1" | tr '\n' ' ' | tr -s ' ' +} + # --- normal: §3 obliges fix-or-answer + the Answer-line convention ---------- @test "§3: obliges fix-or-answer per finding and the Answer-line convention" { @@ -44,20 +52,22 @@ extract_section() { @test "§O3: obliges fix-or-answer per finding and the Answer-line convention" { section="$(extract_section '## (O3)' "$TEMPLATE")" - [[ "$section" == *"fix it, or answer its Question"* ]] - [[ "$section" == *"Answer (r{N})"* ]] - [[ "$section" == *"silence"* ]] - [[ "$section" == *"blocking"* ]] - [[ "$section" == *"Non-blocking"* ]] + flat="$(normalize_ws "$section")" + [[ "$flat" == *"fix it, or answer its Question"* ]] + [[ "$flat" == *"Answer (r{N})"* ]] + [[ "$flat" == *"silence"* ]] + [[ "$flat" == *"blocking"* ]] + [[ "$flat" == *"Non-blocking"* ]] } # --- normal: §O3 obliges the per-finding worker_done --body summary --------- @test "§O3: worker_done --body must summarize per-finding outcomes" { section="$(extract_section '## (O3)' "$TEMPLATE")" - [[ "$section" == *"--body"* ]] - [[ "$section" == *"fixed"* ]] - [[ "$section" == *"stands"* ]] + flat="$(normalize_ws "$section")" + [[ "$flat" == *"--body"* ]] + [[ "$flat" == *"fixed"* ]] + [[ "$flat" == *"stands"* ]] } # --- boundary: §3 stays exactly one physical line (send-keys -l safe) -------