Add a consumption test for the compiled wiki - #6
Conversation
The repo verifies that md2okf produces a well-formed wiki. Nothing verified
the wiki was any use, which is a different claim and the one OKF's own goals
make ("inform how consumption agents should read and traverse it").
The test gives an agent a badly written paragraph and nothing but the wiki:
a new pi-consume service mounts okf/ read-only and does not mount md/ at all,
so the source documents are out of reach. Each reply must end in a json block
naming its changes and the page each came from.
Grading is three string/filesystem assertions, no model in the loop: the edit
landed, the citation resolves, and the cited page really contains the ruling.
The third is the point. Without it an agent could produce the right edit from
memory, attach a plausible path, and score full marks having never opened the
wiki.
The cases lean on rulings a model cannot guess — the wiki requires the % sign
where most guides spell out "per cent", and forbids sentence-initial
"Hopefully" while conceding it is not a grammatical error. Answering from
priors gets those wrong, which is what gives the test the ability to fail.
Provider and model are passed as flags rather than read from settings.json, so
a run states which model it exercised and leaves repo config untouched.
The first real run failed impact-as-verb, and the agent was right while the fixture was wrong. It cited chapter 1, which says outright that "to impact" annoys enough readers that you should write "to have an impact on" — quoted it verbatim and applied exactly that fix. The fixture only accepted chapter 9's wording, where the same prohibition appears in a list of nouns not to verb. So `grounding` now takes a list and any one entry grounds the citation. This widens what counts as a correct source; it does not weaken the check that a source is required. Re-running the synthetic bad inputs confirms the hallucinated-citation case still fails. Also fixes the driver: compose interpolates every service in the file, so the sibling `pi` service's OPENROUTER_API_KEY fail-fast aborted a consumption run that never touches OpenRouter. The driver now supplies a placeholder purely to satisfy interpolation, after the real provider key check.
📝 WalkthroughWalkthroughAdds an isolated ChangesOKF consumption test
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/house-style/grade.py (1)
42-51: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConstrain
resolve()to stay inside the wiki directory.
relcan still contain../segments after stripping the leading slash and optionalokf/prefix, sowiki / relcan resolve outsidewiki. Sincecitationoriginates from model output, a hallucinated or malformed citation could reference a file elsewhere on the host filesystem.is_file()would treat that as a valid, resolved citation, and the "grounded" check would then read that external file's content.🔒 Proposed fix
def resolve(wiki, citation): """Map a bundle-absolute citation to a file on disk, or None.""" if not isinstance(citation, str) or not citation.strip(): return None rel = citation.strip().lstrip("/") # Tolerate an okf/ prefix even though the skill asks for bundle-absolute. if rel.startswith(f"{wiki.name}/"): rel = rel[len(wiki.name) + 1:] - path = wiki / rel - return path if path.is_file() else None + wiki_root = wiki.resolve() + candidate = (wiki / rel).resolve() + if candidate != wiki_root and wiki_root not in candidate.parents: + return None + return candidate if candidate.is_file() else None🤖 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 `@tests/house-style/grade.py` around lines 42 - 51, Update resolve() to normalize the candidate path and verify it remains within wiki before accepting it. After constructing wiki / rel, use the path’s resolved form and require it to be equal to wiki or have wiki as its parent, then apply the existing is_file() check; return None for traversal outside the wiki directory.scripts/test-house-style.sh (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an array instead of unquoted word-splitting; the
caseblock has no default.Line 73's
for id in ${ids}; dorelies on unquoted expansion to word-split, which ShellCheck flags (SC2086). Build the id list as an array instead. Separately, thecase "${provider}" in ... esacat Lines 47-50 has no default branch: an unrecognized--provider/PI_PROVIDERvalue silently skips the credential check, so a typo produces a confusing auth failure deep inside the container run instead of a clearfail_inframessage.🔧 Proposed fix
-ids="$(python3 -c " +mapfile -t ids < <(python3 -c " import json,sys for c in json.load(open('${cases_file}'))['cases']: print(c['id']) -")" +") ... -for id in ${ids}; do +for id in "${ids[@]}"; docase "${provider}" in litellm) [[ -n "${LITELLM_API_KEY:-}" ]] || fail_infra "LITELLM_API_KEY is not set (provider=litellm)." ;; openrouter) [[ -n "${OPENROUTER_API_KEY:-}" ]] || fail_infra "OPENROUTER_API_KEY is not set (provider=openrouter)." ;; +*) fail_infra "Unrecognized provider: ${provider}" ;; esacAs per path instructions, prefer "arrays over constructed command strings" and require "ShellCheck-clean code."
Also applies to: 64-73
🤖 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 `@scripts/test-house-style.sh` around lines 47 - 50, Update the id iteration near the existing `for id in ${ids}` loop to construct and iterate an array, avoiding unquoted word splitting and keeping the script ShellCheck-clean. Add a default branch to the `case "${provider}"` validation that calls `fail_infra` with a clear unsupported-provider message, while preserving the existing credential checks for `litellm` and `openrouter`.Source: Path instructions
🤖 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 `@docs/superpowers/specs/2026-07-31-okf-consumption-test-design.md`:
- Around line 120-128: Update the Components table entry for the house-style
fixtures to reference cases.json instead of cases.yaml, leaving the surrounding
component paths and descriptions unchanged.
In `@scripts/test-house-style.sh`:
- Around line 41-42: Update the compiled_pages check and the docker compose
build flow in the test-house-style script to explicitly catch failures and route
them through fail_infra with clear stderr messages, preserving the documented
infrastructure-failure exit code 2. Ensure find errors are not suppressed or
allowed to terminate via set -e before the existing zero-page validation, and
ensure build failures likewise invoke fail_infra instead of propagating Docker’s
exit status.
- Around line 84-90: Remove the unconditional failure suppression from the
docker compose invocation in the per-case loop of scripts/test-house-style.sh.
Ensure failures from the pi-consume run, including container, gateway, network,
or other infrastructure errors, propagate to the script’s infrastructure-failure
handling and produce the designated exit status 2 instead of being passed to
grade.py as malformed output.
---
Nitpick comments:
In `@scripts/test-house-style.sh`:
- Around line 47-50: Update the id iteration near the existing `for id in
${ids}` loop to construct and iterate an array, avoiding unquoted word splitting
and keeping the script ShellCheck-clean. Add a default branch to the `case
"${provider}"` validation that calls `fail_infra` with a clear
unsupported-provider message, while preserving the existing credential checks
for `litellm` and `openrouter`.
In `@tests/house-style/grade.py`:
- Around line 42-51: Update resolve() to normalize the candidate path and verify
it remains within wiki before accepting it. After constructing wiki / rel, use
the path’s resolved form and require it to be equal to wiki or have wiki as its
parent, then apply the existing is_file() check; return None for traversal
outside the wiki directory.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 555324cc-cdb6-4af7-a1ef-85ebdf7d5fff
📒 Files selected for processing (8)
.gitignoredocs/superpowers/specs/2026-07-31-okf-consumption-test-design.mdpi/container/agent/skills/apply-house-style/SKILL.mdpi/container/compose.yamlpi/sandbox/files/home/.pi/agent/skills/apply-house-style/SKILL.mdscripts/test-house-style.shtests/house-style/cases.jsontests/house-style/grade.py
| ## Components | ||
|
|
||
| | Path | Purpose | | ||
| | --- | --- | | ||
| | `pi/{container,sandbox}/…/skills/apply-house-style/SKILL.md` | the consumption task, one copy per runtime, aligned by hand | | ||
| | `tests/house-style/cases.yaml` | fixtures: input, assertions, expected page, grounding | | ||
| | `tests/house-style/grade.py` | the three assertions, table output, exit code | | ||
| | `scripts/test-house-style.sh` | driver: one Pi run per case, then grade | | ||
| | `pi/container/compose.yaml` | adds the `pi-consume` service | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix stale fixture-file extension in the Components table.
Line 125 lists tests/house-style/cases.yaml. The actual fixtures file added in this PR is tests/house-style/cases.json. A reader following this table to find or edit fixtures looks for the wrong file.
📝 Proposed fix
-| `tests/house-style/cases.yaml` | fixtures: input, assertions, expected page, grounding |
+| `tests/house-style/cases.json` | fixtures: input, assertions, expected page, grounding |As per path instructions, "flag commands, paths, options, and examples that this change has made wrong or stale."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Components | |
| | Path | Purpose | | |
| | --- | --- | | |
| | `pi/{container,sandbox}/…/skills/apply-house-style/SKILL.md` | the consumption task, one copy per runtime, aligned by hand | | |
| | `tests/house-style/cases.yaml` | fixtures: input, assertions, expected page, grounding | | |
| | `tests/house-style/grade.py` | the three assertions, table output, exit code | | |
| | `scripts/test-house-style.sh` | driver: one Pi run per case, then grade | | |
| | `pi/container/compose.yaml` | adds the `pi-consume` service | | |
| ## Components | |
| | Path | Purpose | | |
| | --- | --- | | |
| | `pi/{container,sandbox}/…/skills/apply-house-style/SKILL.md` | the consumption task, one copy per runtime, aligned by hand | | |
| | `tests/house-style/cases.json` | fixtures: input, assertions, expected page, grounding | | |
| | `tests/house-style/grade.py` | the three assertions, table output, exit code | | |
| | `scripts/test-house-style.sh` | driver: one Pi run per case, then grade | | |
| | `pi/container/compose.yaml` | adds the `pi-consume` service | |
🤖 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/superpowers/specs/2026-07-31-okf-consumption-test-design.md` around
lines 120 - 128, Update the Components table entry for the house-style fixtures
to reference cases.json instead of cases.yaml, leaving the surrounding component
paths and descriptions unchanged.
Source: Path instructions
| compiled_pages="$(find okf -name '*.md' ! -name '.okflintrc.json' 2>/dev/null | wc -l | tr -d ' ')" | ||
| [[ "${compiled_pages}" -gt 0 ]] || fail_infra "No compiled wiki under okf/ — run a compile first." |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
find failure can bypass fail_infra and give the wrong exit code.
set -euo pipefail is active. If okf is missing (or find otherwise fails), pipefail makes the pipeline's exit status equal find's non-zero status, and this un-guarded assignment triggers set -e immediately — before Line 42's explicit -gt 0 check runs. The script then exits with find's exit code (typically 1) instead of the documented 2 for infrastructure trouble, and 2>/dev/null on find means no message reaches stderr at all. The docker compose ... build on Line 62 has the same class of gap: a build failure exits the script via set -e with Docker's own exit code, not the documented 2.
🔧 Proposed fix
-compiled_pages="$(find okf -name '*.md' ! -name '.okflintrc.json' 2>/dev/null | wc -l | tr -d ' ')"
-[[ "${compiled_pages}" -gt 0 ]] || fail_infra "No compiled wiki under okf/ — run a compile first."
+[[ -d okf ]] || fail_infra "No compiled wiki under okf/ — run a compile first."
+compiled_pages="$(find okf -name '*.md' ! -name '.okflintrc.json' | wc -l | tr -d ' ')"
+[[ "${compiled_pages}" -gt 0 ]] || fail_infra "No compiled wiki under okf/ — run a compile first."As per path instructions, "validate prerequisites early and exit non-zero with a clear message on stderr" and avoid "silently masked failures."
Also applies to: 62-62
🤖 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 `@scripts/test-house-style.sh` around lines 41 - 42, Update the compiled_pages
check and the docker compose build flow in the test-house-style script to
explicitly catch failures and route them through fail_infra with clear stderr
messages, preserving the documented infrastructure-failure exit code 2. Ensure
find errors are not suppressed or allowed to terminate via set -e before the
existing zero-page validation, and ensure build failures likewise invoke
fail_infra instead of propagating Docker’s exit status.
Source: Path instructions
| docker compose -f "${compose_file}" run --rm -T pi-consume \ | ||
| -xt bash \ | ||
| --provider "${provider}" \ | ||
| --model "${model}" \ | ||
| -p "Load the apply-house-style skill: read /home/node/.pi/agent/skills/apply-house-style/SKILL.md, then follow it to apply house style to this paragraph: ${paragraph}" \ | ||
| </dev/null >"${outdir}/${id}.txt" 2>&1 || true | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Per-case run failures are masked, so infra trouble can look like a grading failure.
The || true on Line 89 means any failure of the docker compose run for a single case (container startup failure, gateway timeout, network error) is swallowed. grade.py then reads whatever partial/garbled output landed in the transcript, most likely reports "no parseable json block", and the case counts as a normal grading failure contributing to exit 1. The design doc explicitly states a gateway error belongs to the 2 (infrastructure trouble) category, and that "Infrastructure failure must never read as 'the wiki is bad'." As written, an intermittent gateway error during one case's run reads exactly as a bad wiki.
🔧 Proposed direction
+ run_status=0
docker compose -f "${compose_file}" run --rm -T pi-consume \
-xt bash \
--provider "${provider}" \
--model "${model}" \
-p "Load the apply-house-style skill: read /home/node/.pi/agent/skills/apply-house-style/SKILL.md, then follow it to apply house style to this paragraph: ${paragraph}" \
- </dev/null >"${outdir}/${id}.txt" 2>&1 || true
+ </dev/null >"${outdir}/${id}.txt" 2>&1 || run_status=$?
+ [[ ${run_status} -eq 0 ]] || fail_infra "pi-consume run for case '${id}' exited ${run_status} — see ${outdir}/${id}.txt"
doneAs per path instructions, avoid "silently masked failures."
🤖 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 `@scripts/test-house-style.sh` around lines 84 - 90, Remove the unconditional
failure suppression from the docker compose invocation in the per-case loop of
scripts/test-house-style.sh. Ensure failures from the pi-consume run, including
container, gateway, network, or other infrastructure errors, propagate to the
script’s infrastructure-failure handling and produce the designated exit status
2 instead of being passed to grade.py as malformed output.
Source: Path instructions
|
|
||
| ## Procedure | ||
|
|
||
| 1. **Find the rulings.** Search `okf/` for the words and constructions used in |
There was a problem hiding this comment.
Much of style guides are rules for specific words. In these cases, the CLI tools vale (active development, 5.7k stars) or qmd (active development, 28.5k stars) in addition to okf might help the Pi agent.
The harder problem are soft, less specific rules. Even if you can load thousands of lines of md into context, it will most likely lead to context rot i.e. the rules are overlooked and not applied.
2 participants
Why
The repo verifies that
md2okfproduces a well-formed wiki: a book compiles,okf-lintpasses, pages carry the right frontmatter. Nothing verified the wikiwas any use, which is a different claim — and the one OKF's own goals make:
How it works
An agent is given one badly written paragraph and nothing but the wiki. A new
pi-consumeCompose service mountsokf/read-only and does not mountmd/at all, so the source documents are out of reach and the answer has to come
from the compiled pages. Each reply ends in a JSON block naming every change and
the page it came from.
Grading is three assertions with no model in the loop — every check is a string
or filesystem operation, so a run is reproducible and a failure is inspectable:
The third is the point. Without it an agent could produce the right edit from
memory, attach a plausible-looking path, and score full marks having never
opened the wiki. The grader was self-tested against five deliberately bad
inputs — hallucinated citation, unapplied edit, missing JSON block, unresolvable
path, absent transcript — and rejects all five.
Guarding against prior knowledge
A capable model already knows roughly what Economist house style is, so a good
rewrite proves nothing on its own. Cases therefore lean on rulings that cannot
be derived from general knowledge, several of which invert the usual
convention — the wiki requires the
%sign where most guides spell out "percent", and forbids sentence-initial "Hopefully" while conceding it is not a
grammatical error. Answering from priors gets those wrong, which is what gives
the test the ability to fail.
There is deliberately no A/B control arm. It would quantify the wiki's
contribution but doubles the runs; the counterintuitive rulings already provide
falsifiability. Recorded as a limitation in the spec, not overlooked.
Result
8/8 against a wiki compiled from a 382 KB style guide. The most convincing part
was not the score: on the percentage case the agent made an unprompted extra
change, rewriting "last year" as "in the past year" and citing a ruling that
last year, in 2023, means 2022. That ruling was not in the fixture and not in
the prompt — it was read off a page.
The first run scored 7/8, and the agent was right while the fixture was wrong:
it cited chapter 1, which says outright that to impact should be to have an
impact on, while the fixture only accepted chapter 9's phrasing of the same
prohibition.
groundingnow takes a list. That widens what counts as a validsource without weakening the requirement for one — the synthetic bad inputs were
re-run afterwards and still fail.
Notes for review
two-independent-runtimes rule in
AGENTS.md.--provider/--modelflags rather thanread from
settings.json, so a run states which model it exercised and leavesrepo config untouched. Override with
PI_PROVIDER/PI_MODEL.OPENROUTER_API_KEYbefore invoking Compose.This is not sloppiness: Compose interpolates every service in the file, so the
sibling
piservice's fail-fast would otherwise abort a consumption run thatnever touches OpenRouter. The real provider key is checked first, and exits 2
if missing.
0allpassed,
1a case failed,2could not run. A missing wiki or absent key isnever reported as "the wiki is bad".
tests/house-style/out/, which is gitignored.docs/superpowers/specs/2026-07-31-okf-consumption-test-design.md.Verification
make lint— cleanmake validate—VALIDgroundingstring confirmed present at its expected page before runninglitellm, modelgemini-3.1-pro-previewNot covered
Navigability. The container ships
ripgrepandfd, so the agent searchesrather than traversing
index.mdfiles. This test says nothing about whetherthe index structure works — a separate test could deny the search tools and
require traversal from the root index.
Summary by CodeRabbit