Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
6 changes: 4 additions & 2 deletions INDEX.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
47 changes: 44 additions & 3 deletions skills/orchestrate/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -476,14 +476,55 @@ 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 <impl_task> --terminal <handle>` 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 <wt> diff <integ>...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 <wt> diff
<integ>...HEAD`) — write the result to `reviews/<task>-rN.md` from
`templates/review-report.md`:

1. **Plan conformance** — diff vs. the plan's decision→page map and the
brief's `<scope_boundaries>` / `<out_of_scope>`; 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/<task>-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
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: <diff signal in lens vocabulary — e.g. "new CLI flag, no version check">
directive: <reviewer's action on that signal — e.g. "confirm the flag exists in the deployed toolchain version">
why: <why the miss happened — e.g. "lens 3 exists for exactly this, the first pass skipped it">
evidence: reviews/<task>-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
Expand Down
38 changes: 38 additions & 0 deletions skills/orchestrate/templates/review-report.md
Original file line number Diff line number Diff line change
@@ -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 — <what was checked>` or `findings: F1, F2` or `not run — <why>` |
| 2. Wiki re-route from the diff | `clean — <what was checked>` or `findings: F1, F2` or `not run — <why>` |
| 3. Execution-environment reality | `clean — <what was checked>` or `findings: F1, F2` or `not run — <why>` |
| 4. Multi-object write ordering | `clean — <what was checked>` or `findings: F1, F2` or `not run — <why>` |

## Findings

Each finding must state a concrete failure scenario. If it cannot, it belongs
in **Non-blocking** below, not here.

### F1

- **Observation** — <what the diff does, quoted at `file:line`>
- **Failure scenario** — <the concrete inputs/state that make it wrong>
- **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** — <what the diff does, quoted at `file:line`>
- **Question** — <"why this way?">
11 changes: 8 additions & 3 deletions skills/orchestrate/templates/session-prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: <reason>` 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

Expand Down Expand Up @@ -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: <reason>` 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 "<what changed since r{N}, what remains>" --task-id {ORCA_TASK_ID} --dispatch-id {ORCA_DISPATCH_ID} --outcome succeeded --files-modified "<csv>" --json`
`orca orchestration send --type worker_done --subject "impl_done: {TASK} r{N}" --body "<per-finding outcomes, e.g. F1 fixed, F2 stands: short reason>" --task-id {ORCA_TASK_ID} --dispatch-id {ORCA_DISPATCH_ID} --outcome succeeded --files-modified "<csv>" --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
Expand Down
205 changes: 205 additions & 0 deletions tests/orchestrate-insight-emission.bats
Original file line number Diff line number Diff line change
@@ -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() { # <path> <content>
local content_json
content_json="$(printf '%s' "$2" | _json_string)"
printf '{"message":{"role":"assistant","content":%s}}\n' "$content_json" > "$1"
}

_run_harvest() { # <session_id> <transcript>
printf '{"cwd":"%s","session_id":"%s","transcript_path":"%s"}' "$WORK" "$1" "$2" | \
HOME="$HOME" node "$HARVEST"
}

_queue_lines() { # <session_id>
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/<task>-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"
}
Loading
Loading