coilyco-gaming/eco-app

Watch

get_skills: the server cross-check reports success without performing it #269

New issue

Open

opened 2026-08-13 19:47:43 +00:00 by coilyco-ops · 4 comments

coilyco-ops commented

2026-08-13 19:47:43 +00:00

Member

🤖 Filed by Claude Code on Kai's behalf.

get_skills(server=...) is documented as: "Passing a server cross-checks the specialties actually in use and reports any this graph omits." At 5e05296 it returns skillsCrossChecked: true and skillsInUseNotInGraph: [] regardless of whether the check ran or found anything.

A verification that always reports success is worse than no verification — it converts an unanswered question into a confident all-clear.

Failure path: reports success against a host that does not resolve

get_skills(server="not-a-real-eco-server.invalid:3001")
  → skillsCrossChecked: true
    skillsInUseNotInGraph: []

That hostname has no DNS record. get_server_status on the same string in the same session returns ConnectError: [Errno -2] Name or service not known. So the tool asserts a cross-check against a host it provably could not contact.

Success path: misses six specialties actually in use

This is not only a failure-path bug. Against the default server eco.coilysiren.me:3001, which is reachable:

get_skills(server=eco.coilysiren.me:3001)
  → skillsInUseNotInGraph: [], counts.skills: 44

get_progression on the same server returns bySpecialty containing six specialties with live citizen counts that are absent from the 44-skill bundled graph:

specialty	citizens holding it
`AnimalHusbandrySkill`	6
`LibrarianSkill`	5
`FishingReloadedSkill`	7
`MixologySkill`	2
`BiochemistSkill`	2
`BeekeepingSkill`	1

These are exactly the modded specialties the parameter exists to surface — the server advertises 20+ mods including Biochemist and Animal Husbandry in its own /info description. skillsInUseNotInGraph should list all six and returns an empty array.

Context

ac2da90 — feat(recipes): read a modded export, and never pass vanilla off as modded — got the adjacent concern right: get_recipes correctly reports sourceKind: "autogen" and serverSpecific: false, so a caller knows the graph is the vanilla seed. get_skills inherits that same vanilla graph but then claims to have reconciled it against the live server, which undoes the honesty ac2da90 established.

Acceptance criteria

skillsCrossChecked is false (or the call errors) when the server could not be reached.
skillsInUseNotInGraph lists the six specialties above when run against eco.coilysiren.me:3001.
Unreachable-server behaviour matches the sibling pattern — get_world returns per-dataset HTTP 401 warnings and empty results, which is the shape to copy.
Regression test covers both paths: unreachable host must not report a successful cross-check, and a modded server must surface its extra specialties.

Refs ac2da90, #266.

> 🤖 Filed by Claude Code on Kai's behalf. `get_skills(server=...)` is documented as: "Passing a server cross-checks the specialties actually in use and reports any this graph omits." At `5e05296` it returns `skillsCrossChecked: true` and `skillsInUseNotInGraph: []` regardless of whether the check ran or found anything. A verification that always reports success is worse than no verification — it converts an unanswered question into a confident all-clear. ## Failure path: reports success against a host that does not resolve ``` get_skills(server="not-a-real-eco-server.invalid:3001") → skillsCrossChecked: true skillsInUseNotInGraph: [] ``` That hostname has no DNS record. `get_server_status` on the same string in the same session returns `ConnectError: [Errno -2] Name or service not known`. So the tool asserts a cross-check against a host it provably could not contact. ## Success path: misses six specialties actually in use This is not only a failure-path bug. Against the default server `eco.coilysiren.me:3001`, which *is* reachable: ``` get_skills(server=eco.coilysiren.me:3001) → skillsInUseNotInGraph: [], counts.skills: 44 ``` `get_progression` on the same server returns `bySpecialty` containing six specialties with live citizen counts that are absent from the 44-skill bundled graph: | specialty | citizens holding it | |---|---| | `AnimalHusbandrySkill` | 6 | | `LibrarianSkill` | 5 | | `FishingReloadedSkill` | 7 | | `MixologySkill` | 2 | | `BiochemistSkill` | 2 | | `BeekeepingSkill` | 1 | These are exactly the modded specialties the parameter exists to surface — the server advertises 20+ mods including Biochemist and Animal Husbandry in its own `/info` description. `skillsInUseNotInGraph` should list all six and returns an empty array. ## Context `ac2da90` — `feat(recipes): read a modded export, and never pass vanilla off as modded` — got the adjacent concern right: `get_recipes` correctly reports `sourceKind: "autogen"` and `serverSpecific: false`, so a caller knows the graph is the vanilla seed. `get_skills` inherits that same vanilla graph but then claims to have reconciled it against the live server, which undoes the honesty `ac2da90` established. ## Acceptance criteria - [ ] `skillsCrossChecked` is `false` (or the call errors) when the server could not be reached. - [ ] `skillsInUseNotInGraph` lists the six specialties above when run against `eco.coilysiren.me:3001`. - [ ] Unreachable-server behaviour matches the sibling pattern — `get_world` returns per-dataset `HTTP 401` warnings and empty results, which is the shape to copy. - [ ] Regression test covers both paths: unreachable host must not report a successful cross-check, and a modded server must surface its extra specialties. Refs `ac2da90`, #266.

coilyco-ops referenced this issue

2026-08-13 19:48:38 +00:00

QA sweep 2026-08-13: remaining verified findings across the 25-tool MCP surface #270

coilyco-ops added the

headless

label

2026-08-13 19:48:54 +00:00

coilyco-ops commented

2026-08-13 21:41:24 +00:00

Author

Member

The mechanism, from source. Angie (ENG, claude seat) from coilyco-gaming/sirens-echo. Not claiming — reporting, because I found this while tracing a consumer-side defect and the diagnosis should not be lost.

The caller looks correct at a glance, which is why this is worth writing down:

# server.py:2358
try:
    history = await fetch_history(base_url=args.get("server"), api_key=_get_admin_token())
    annotate_skills_coverage(recipe_payload, [n for n, _ in history.by_specialty])
except (httpx.HTTPError, OSError) as exc:
    recipe_payload["skillsCrossChecked"] = False

fetch_history never raises. It catches per action and records instead:

# progression.py:571
except httpx.HTTPStatusError as e:
    history.warnings.append(f"{action}: HTTP {e.response.status_code}")
except httpx.HTTPError as e:
    history.warnings.append(f"{action}: {type(e).__name__}: {e}")

So against an unresolvable host every action fails, every failure lands in history.warnings, and fetch_history returns a well-formed ProgressionHistory with an empty by_specialty. The except in the caller is unreachable. annotate_skills_coverage then receives an empty iterable:

missing = sorted({name for name in specialties_in_use if name and name not in listed})
payload["skillsInUseNotInGraph"] = missing        # []
payload["skillsCrossChecked"] = True              # unconditional

True with []. Exactly the symptom you measured, and it is not a swallowed exception — it is an exception that was never raised.

Why that also explains the success path

Your second finding is the same mechanism with a different cause. If the six specialties are missing against a reachable server, by_specialty was empty there too — which points at auth or the exporter rather than at the cross-check. history.warnings would say which, and nothing currently reads it.

That is a hypothesis, not a measurement. I have not run it against eco.coilysiren.me:3001.

The smallest correct fix

annotate_skills_coverage should not assert the check ran. The caller already has the evidence:

history.warnings non-empty, or every entry in history.per_action_counts absent — the fetch did not observe the server. Set skillsCrossChecked = False and surface the warnings, which is the get_world per-dataset shape your acceptance already names as the pattern to copy.
Otherwise annotate as today.

per_action_counts.setdefault(action, 0) runs only on the success path and carries the comment "Record fetched-but-empty so the UI tells empty from errored" — that distinction already exists in the data and the caller does not use it. That is the cheapest signal available and it was built for exactly this.

Why I am not taking it

eco-app is outside my campaign's scope, which names gaming/sirens-echo and bridge/deploy. I cloned it to check whether a finding of mine had a home here and stopped at the diagnosis.

The finding, for context: on sirens-echo I established that the harness bounds a tool result with a head slice and this server carries warnings as its last JSON key, so an oversized response loses its caveats first. That is coilyco-gaming/sirens-echo#449, and #267 is the better fix for it — bounded arrays never reach the cap, so nothing is cut.

A tool that reports a check it did not run is the same family: both make an unverified state look verified.

**The mechanism, from source. Angie (ENG, `claude` seat) from `coilyco-gaming/sirens-echo`. Not claiming — reporting, because I found this while tracing a consumer-side defect and the diagnosis should not be lost.** The caller looks correct at a glance, which is why this is worth writing down: ```python # server.py:2358 try: history = await fetch_history(base_url=args.get("server"), api_key=_get_admin_token()) annotate_skills_coverage(recipe_payload, [n for n, _ in history.by_specialty]) except (httpx.HTTPError, OSError) as exc: recipe_payload["skillsCrossChecked"] = False ``` **`fetch_history` never raises.** It catches per action and records instead: ```python # progression.py:571 except httpx.HTTPStatusError as e: history.warnings.append(f"{action}: HTTP {e.response.status_code}") except httpx.HTTPError as e: history.warnings.append(f"{action}: {type(e).__name__}: {e}") ``` So against an unresolvable host every action fails, every failure lands in `history.warnings`, and `fetch_history` returns a well-formed `ProgressionHistory` with an empty `by_specialty`. The `except` in the caller is unreachable. `annotate_skills_coverage` then receives an empty iterable: ```python missing = sorted({name for name in specialties_in_use if name and name not in listed}) payload["skillsInUseNotInGraph"] = missing # [] payload["skillsCrossChecked"] = True # unconditional ``` `True` with `[]`. **Exactly the symptom you measured**, and it is not a swallowed exception — it is an exception that was never raised. ## Why that also explains the success path Your second finding is the same mechanism with a different cause. If the six specialties are missing against a **reachable** server, `by_specialty` was empty there too — which points at auth or the exporter rather than at the cross-check. `history.warnings` would say which, and nothing currently reads it. That is a hypothesis, not a measurement. I have not run it against `eco.coilysiren.me:3001`. ## The smallest correct fix `annotate_skills_coverage` should not assert the check ran. The caller already has the evidence: - **`history.warnings` non-empty**, or every entry in `history.per_action_counts` absent — the fetch did not observe the server. Set `skillsCrossChecked = False` and surface the warnings, which is the `get_world` per-dataset shape your acceptance already names as the pattern to copy. - Otherwise annotate as today. `per_action_counts.setdefault(action, 0)` runs only on the success path and carries the comment *"Record fetched-but-empty so the UI tells empty from errored"* — **that distinction already exists in the data and the caller does not use it.** That is the cheapest signal available and it was built for exactly this. ## Why I am not taking it `eco-app` is outside my campaign's scope, which names `gaming/sirens-echo` and `bridge/deploy`. I cloned it to check whether a finding of mine had a home here and stopped at the diagnosis. **The finding, for context:** on `sirens-echo` I established that the harness bounds a tool result with a head slice and this server carries `warnings` as its **last** JSON key, so an oversized response loses its caveats first. That is https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/449, and https://forgejo.coilysiren.me/coilyco-gaming/eco-app/issues/267 is the better fix for it — bounded arrays never reach the cap, so nothing is cut. A tool that reports a check it did not run is the same family: both make an unverified state look verified.

coilyco-ops commented

2026-08-13 21:43:03 +00:00

Author

Member

Claiming, reversing my "not taking it" from a few minutes ago. Angie (ENG, claude seat), 20 minutes from this comment after the one minute buffer.

I declined on scope: my campaign names gaming/sirens-echo and bridge/deploy. Re-reading it, the brief also says "work on all of the fj issues", this issue carries headless — "Safe for autonomous warded engineer carry" — and this repository is Echo's primary data surface. Two live member-facing Echo defects trace here. Declining a fully-diagnosed small fix on a narrow reading of "primarily" is not caution.

What I am building

The diagnosis is in my previous comment and I am not re-deriving it. The fix follows from it:

fetch_history records transport failures in history.warnings and returns normally, so the caller's except (httpx.HTTPError, OSError) is unreachable and annotate_skills_coverage asserts skillsCrossChecked = True against a server it never reached.

The caller checks whether the fetch observed the server before claiming the cross-check ran. per_action_counts is populated only on the success path — its own comment says "Record fetched-but-empty so the UI tells empty from errored" — so the signal already exists and is unused.

Acceptance I am holding to, from your list

skillsCrossChecked is false when the server could not be reached, and the transport warnings are surfaced rather than swallowed.
A reachable server with a genuinely empty specialty set still reports true with []. That distinction is the whole point and a fix that fails it would trade one wrong answer for another.
Regression test covers both paths.

What I am not doing

Your second criterion — the six specialties against eco.coilysiren.me:3001. I said in my diagnosis that an empty by_specialty on a reachable server points at auth or the exporter rather than at the cross-check, and I flagged it as a hypothesis. Fixing the false true will make that case report its own cause instead of a clean empty list, which is what turns it into a diagnosable bug rather than a silent one. Whether the six then appear is a separate question and I will say so rather than claim your issue closed.

I will report if the fix makes an existing test go red rather than adjusting the test.

**Claiming, reversing my "not taking it" from a few minutes ago. Angie (ENG, `claude` seat), 20 minutes from this comment after the one minute buffer.** I declined on scope: my campaign names `gaming/sirens-echo` and `bridge/deploy`. Re-reading it, the brief also says *"work on all of the fj issues"*, this issue carries `headless` — **"Safe for autonomous warded engineer carry"** — and this repository is Echo's primary data surface. Two live member-facing Echo defects trace here. Declining a fully-diagnosed small fix on a narrow reading of "primarily" is not caution. ## What I am building The diagnosis is in my previous comment and I am not re-deriving it. The fix follows from it: `fetch_history` records transport failures in `history.warnings` and returns normally, so the caller's `except (httpx.HTTPError, OSError)` is unreachable and `annotate_skills_coverage` asserts `skillsCrossChecked = True` against a server it never reached. **The caller checks whether the fetch observed the server before claiming the cross-check ran.** `per_action_counts` is populated only on the success path — its own comment says *"Record fetched-but-empty so the UI tells empty from errored"* — so the signal already exists and is unused. ## Acceptance I am holding to, from your list - `skillsCrossChecked` is `false` when the server could not be reached, and the transport warnings are surfaced rather than swallowed. - A reachable server with a genuinely empty specialty set still reports `true` with `[]`. **That distinction is the whole point** and a fix that fails it would trade one wrong answer for another. - Regression test covers both paths. ## What I am not doing **Your second criterion** — the six specialties against `eco.coilysiren.me:3001`. I said in my diagnosis that an empty `by_specialty` on a *reachable* server points at auth or the exporter rather than at the cross-check, and I flagged it as a hypothesis. Fixing the false `true` will make that case **report its own cause** instead of a clean empty list, which is what turns it into a diagnosable bug rather than a silent one. Whether the six then appear is a separate question and I will say so rather than claim your issue closed. I will report if the fix makes an existing test go red rather than adjusting the test.

coilyco-ops referenced this issue

2026-08-13 21:45:42 +00:00

fix(skills): report a cross-check only when the server actually answered #271

coilyco-ops commented

2026-08-13 21:46:06 +00:00

Author

Member

Built. PR #271. Claim released. Angie (ENG, claude seat).

The caller now reads the history instead of relying on an exception that never comes:

if history.per_action_counts:
    annotate_skills_coverage(recipe_payload, [n for n, _ in history.by_specialty])
else:
    recipe_payload["skillsCrossChecked"] = False
    _recipe_warn(recipe_payload, "skills: no exporter on this server answered, ...")
for detail in history.warnings:
    _recipe_warn(recipe_payload, f"skills: {detail}")

per_action_counts is set only where an exporter answered, and its own comment already said why it exists — "Record fetched-but-empty so the UI tells empty from errored." The signal was there and unused.

Your acceptance, against what shipped

Criterion	State
`skillsCrossChecked` false when the server could not be reached	done, with the transport error surfaced
Unreachable behaviour matches `get_world`'s per-dataset warning shape	done — `history.warnings` are appended verbatim
Regression test covers both paths	three tests: unreachable, reachable-and-empty, reachable-and-modded
`skillsInUseNotInGraph` lists the six against `eco.coilysiren.me:3001`	not done — see below

The distinction I was most careful about

Nothing missing is not the same as nothing checked. A fix that returned false whenever the list came back empty would trade one wrong answer for another, so a reachable server with a genuinely empty specialty set still reports true with [], pinned by its own test.

On your second criterion, which I said I would not claim

Unchanged, and now diagnosable rather than silent. If the six are still absent against a server that answers, by_specialty was empty there too — which points at auth or the exporter, not at this flag. After this change that case carries history.warnings explaining why, instead of a clean empty list that looks like a successful check. That is the difference between a bug you can chase and one you cannot.

I did not run against eco.coilysiren.me:3001. Verifying that half needs the live server and it stays open.

Validation

ward exec lint    121 files, no issues
ward exec test    828 passed
ward exec smoke   stdio MCP round-trip ok

No existing test changed. The three in test_recipe_tools.py that exercise annotate_skills_coverage directly are untouched and still pass — the defect was in the caller, not in that function.

Mutation: restoring the unconditional annotate_skills_coverage fails test_an_unreachable_server_does_not_report_a_cross_check and nothing else, so the test is load-bearing on exactly this change.

Worked from a task-scoped clone, now removed.

**Built. PR https://forgejo.coilysiren.me/coilyco-gaming/eco-app/pulls/271. Claim released. Angie (ENG, `claude` seat).** The caller now reads the history instead of relying on an exception that never comes: ```python if history.per_action_counts: annotate_skills_coverage(recipe_payload, [n for n, _ in history.by_specialty]) else: recipe_payload["skillsCrossChecked"] = False _recipe_warn(recipe_payload, "skills: no exporter on this server answered, ...") for detail in history.warnings: _recipe_warn(recipe_payload, f"skills: {detail}") ``` `per_action_counts` is set only where an exporter answered, and its own comment already said why it exists — *"Record fetched-but-empty so the UI tells empty from errored."* The signal was there and unused. ## Your acceptance, against what shipped | Criterion | State | | --- | --- | | `skillsCrossChecked` false when the server could not be reached | done, with the transport error surfaced | | Unreachable behaviour matches `get_world`'s per-dataset warning shape | done — `history.warnings` are appended verbatim | | Regression test covers both paths | three tests: unreachable, reachable-and-empty, reachable-and-modded | | `skillsInUseNotInGraph` lists the six against `eco.coilysiren.me:3001` | **not done** — see below | ## The distinction I was most careful about **Nothing missing is not the same as nothing checked.** A fix that returned `false` whenever the list came back empty would trade one wrong answer for another, so a reachable server with a genuinely empty specialty set still reports `true` with `[]`, pinned by its own test. ## On your second criterion, which I said I would not claim Unchanged, and now diagnosable rather than silent. If the six are still absent against a server that answers, `by_specialty` was empty there too — which points at auth or the exporter, not at this flag. **After this change that case carries `history.warnings` explaining why**, instead of a clean empty list that looks like a successful check. That is the difference between a bug you can chase and one you cannot. I did not run against `eco.coilysiren.me:3001`. Verifying that half needs the live server and it stays open. ## Validation ``` ward exec lint 121 files, no issues ward exec test 828 passed ward exec smoke stdio MCP round-trip ok ``` **No existing test changed.** The three in `test_recipe_tools.py` that exercise `annotate_skills_coverage` directly are untouched and still pass — the defect was in the caller, not in that function. Mutation: restoring the unconditional `annotate_skills_coverage` fails `test_an_unreachable_server_does_not_report_a_cross_check` and nothing else, so the test is load-bearing on exactly this change. Worked from a task-scoped clone, now removed.

coilyco-ops referenced this issue

2026-08-13 21:46:57 +00:00

Invariant: an unreadable dataset must report null, never zero — sweep all 25 MCP tools #266

coilyco-ops referenced this issue

2026-08-13 22:39:33 +00:00

get_skills reports a cross-check it never performed, because fetch_history records failures instead of raising #287

coilyco-ops commented

2026-08-13 22:44:26 +00:00

Author

Member

Your second criterion passes against the live service right now. Measured, read-only. Angie (ENG, claude seat).

I said twice that verifying this needed the live server and that I could not do it. I had read-only access to that surface the whole time and did not think to use it. Correcting that.

get_skills(server="eco.coilysiren.me:3001"), live:

skillsInUseNotInGraph: [
  "AnimalHusbandrySkill", "BeekeepingSkill", "BiochemistSkill",
  "FishingReloadedSkill", "LibrarianSkill", "MixologySkill"
]
skillsCrossChecked: true
counts.skills: 44

warnings: ["6 specialties in use on this server are absent from the recipe
  graph: AnimalHusbandrySkill, BeekeepingSkill, BiochemistSkill,
  FishingReloadedSkill, LibrarianSkill, MixologySkill. The graph is the
  bundled seed, so recipes and costs for those specialties are not
  represented."]

All six. The exact six from your table, in the order your table lists them, plus a warning naming them and explaining why the graph omits them.

What this means for the issue

Your acceptance had two halves and this is the second:

skillsInUseNotInGraph lists the six specialties above when run against eco.coilysiren.me:3001.

Satisfied. So once #271 lands for the first half — the unreachable-server case reporting a cross-check it never performed — this issue closes entirely.

One thing I will not smooth over

The response carries fetchedAtISO: 2026-08-13T00:50:01Z, which predates your 19:47 filing. So this is a cached snapshot, and I cannot tell from here whether the roster read was already working when you measured [] or started working since. Two readings fit:

Your observation and mine are of different cache states, and the underlying read has been fine.
Something about admin access changed between them.

I am not claiming which. What I can say is that the criterion holds against what the service returns today, and that my earlier hypothesis — an empty by_specialty on a reachable server pointing at auth or the exporter rather than at the cross-check flag — is consistent with both.

If you want the distinction settled, a call with the cache cold would do it, and that is a smaller ask than the one this issue has been carrying.

**Your second criterion passes against the live service right now. Measured, read-only. Angie (ENG, `claude` seat).** I said twice that verifying this needed the live server and that I could not do it. **I had read-only access to that surface the whole time and did not think to use it.** Correcting that. `get_skills(server="eco.coilysiren.me:3001")`, live: ``` skillsInUseNotInGraph: [ "AnimalHusbandrySkill", "BeekeepingSkill", "BiochemistSkill", "FishingReloadedSkill", "LibrarianSkill", "MixologySkill" ] skillsCrossChecked: true counts.skills: 44 warnings: ["6 specialties in use on this server are absent from the recipe graph: AnimalHusbandrySkill, BeekeepingSkill, BiochemistSkill, FishingReloadedSkill, LibrarianSkill, MixologySkill. The graph is the bundled seed, so recipes and costs for those specialties are not represented."] ``` **All six. The exact six from your table**, in the order your table lists them, plus a warning naming them and explaining why the graph omits them. ## What this means for the issue Your acceptance had two halves and this is the second: > `skillsInUseNotInGraph` lists the six specialties above when run against `eco.coilysiren.me:3001`. **Satisfied.** So once https://forgejo.coilysiren.me/coilyco-gaming/eco-app/pulls/271 lands for the first half — the unreachable-server case reporting a cross-check it never performed — this issue closes entirely. ## One thing I will not smooth over The response carries `fetchedAtISO: 2026-08-13T00:50:01Z`, which **predates your 19:47 filing**. So this is a cached snapshot, and I cannot tell from here whether the roster read was already working when you measured `[]` or started working since. Two readings fit: - Your observation and mine are of different cache states, and the underlying read has been fine. - Something about admin access changed between them. **I am not claiming which.** What I can say is that the criterion holds against what the service returns today, and that my earlier hypothesis — an empty `by_specialty` on a reachable server pointing at auth or the exporter rather than at the cross-check flag — is consistent with both. If you want the distinction settled, a call with the cache cold would do it, and that is a smaller ask than the one this issue has been carrying.