From 84a0238ec57d4e3a5c667ef5756ec7498bc60037 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 08:45:26 -0400 Subject: [PATCH 01/12] spec: teach Claude the Portal-report workflow [REPORT-95] Requirements and implementation plan for closing the gap between what cc-data can do with Portal reports and what the guidance tells Claude about them. The tool can create Portal runs, download them and join them to student answers; the shared core both surfaces render was written for the Athena-only world and has only been patched where individual stories touched it, so a researcher asking for class-level results gets steered down the Athena path, which is slower, needs a report authored first, and returns a frozen snapshot. Measured rather than assumed: the phrase "Portal report" is absent from the skill surface, neither surface names a single report slug, and neither mentions --refresh, so the first step of the documented workflow is unexecutable from the guidance alone. What the plan builds, in five steps: - Widening ParseCatalog's identifier pattern to accept hyphens. It lands alone because it changes a regex three shipped guards depend on, and every report slug carries a hyphen, so without it a slug catalog parses as empty and its guard passes while checking nothing. - Two report_type guards, each against the code list that defines the sentence it checks. The run vocabulary and the download vocabulary are different sets, and conflating them is what an earlier draft of this spec did. - The core gaining the Portal family, the live-versus-snapshot rule as a decision rule, a guarded slug catalog, the two dimension entries' missing facts, and the end-to-end recipe from creating a Student ID Mapping run to the final join. - --refresh named on each surface, with six MCP descriptions gaining one named fact each, asserted by tests, since the drift guard checks tool names and never reads description text. - A consistency pass over the researcher guide, whose expected outcome is a small diff or none, since REPORT-94 already gave it a full Portal treatment. Decisions a reviewer needs. The run sentence at core.md:24 keeps its list and gains the fact it lacks, that a Portal run has no report_type and execution is what identifies it: a Portal run's report_type is null on the wire, pinned by portalRunWire and stated as design in the API type, so adding portal to that list would contradict the discriminator the same story teaches on reports_list. portal and recovered are values cc-data assigns locally, and they go on the downloads entry where they belong. The slug catalog says in one sentence that it is not exhaustive, because the Portal offers five aggregate metrics reports whose slugs are in no Go inventory and which no endpoint enumerates. The guard runs one direction only, and what it cannot prove is written beside slugToType rather than in the test: it holds the guidance against cc-data's own copy of the slug list, never against the server's. REPORT-130 was filed to replace that copy with a catalog read at runtime, which is the same reasoning the fetch path already applied when it derived a report's type from execution rather than a slug map. The recipe names the joins but points at the rules that govern them rather than restating them, for the withheld join key and for when materializing is worth it. A correctness rule stated twice in one file with nothing holding the copies together is how the two drift. Command spellings cannot go in the core, which TestCoreNamesNoCommand enforces, so the rule goes in the core and the --refresh spelling in the skill header, matching the split the auth remedy already uses. The recipe was drafted into the real file and run against that guard before being specified. Stacked on REPORT-113, because the recipe points at materialize prose that story adds to the same file. Rebase onto main before retargeting the PR when it merges. --- .../implementation.md | 267 ++++++++++++++++++ .../requirements.md | 249 ++++++++++++++++ 2 files changed, 516 insertions(+) create mode 100644 specs/REPORT-95-portal-reports-guidance/implementation.md create mode 100644 specs/REPORT-95-portal-reports-guidance/requirements.md diff --git a/specs/REPORT-95-portal-reports-guidance/implementation.md b/specs/REPORT-95-portal-reports-guidance/implementation.md new file mode 100644 index 0000000..b0864eb --- /dev/null +++ b/specs/REPORT-95-portal-reports-guidance/implementation.md @@ -0,0 +1,267 @@ +# Implementation Plan: Portal reports in the Claude skill and MCP guidance + +**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-95 +**Requirements Spec**: [requirements.md](requirements.md) +**Status**: **In Development** + +## Shape of the change + +This is a documentation story with three code-shaped constraints, all verified: + +- **The core cannot name a command.** `TestCoreNamesNoCommand` rejects any "`cc-data `" string in `core.md`, so the recipe names operations ("create a `student-id-mapping` run") and the command spellings live in `skill_header.md`. The draft was run against the real guard and passes. +- **The slug guard needs a one-character regex change first.** `leadingNames` excludes hyphens, so a slug catalog parses as empty today and `ParseCatalog` reports success anyway. Verified that widening it leaves all four existing catalogs parsing to identical names. +- **Anything in `core.md` reaches both surfaces by construction; anything in `skill_header.md` or a tool description reaches one.** That decides where each fact goes and which facts need a both-surfaces test. + +Both inputs have landed in this branch's base as of 2026-09-11: it is stacked on `REPORT-113-dataset-materialization`, which carries REPORT-112's merge, so the `core.md` conflict the Precondition anticipated is already resolved and stage 4 has been re-run against the result. When REPORT-113 merges, rebase onto `main` before retargeting this PR, so its diff is this story's work alone; GitHub does not retarget a stacked child on its own, and this repo does not delete branches on merge, so the child is not at risk of being closed. + +--- + +## Widen the catalog identifier pattern + +**Summary**: Makes a hyphenated catalog possible. Independent of everything else and worth landing alone, because it changes a regex three shipped guards depend on. + +**Files affected**: +- `internal/guidance/catalog.go` — `leadingNames` +- `internal/guidance/catalog_test.go` — the identical-parse evidence + +**Estimated diff size**: ~60 lines + +```go +// leadingNames matches the backticked identifiers that open a catalog entry, in either +// markup the guarded files use: a bulleted list item ("- `x`, `y` — ...") or a markdown +// table row ("| `x`, `y` | ..."). Hyphens are allowed because report slugs carry them; +// without that a slug catalog parses as empty and its guard passes while checking nothing. +var leadingNames = regexp.MustCompile("^(?:- |\\| )((?:`[a-z0-9_-]+`(?:, )?)+)") +``` + +The test that matters is not that the new pattern accepts a hyphen, which is true by inspection. It is that the four shipped catalogs parse to the *same* names as before, so this cannot quietly change what the existing guards check. Measured before and after: core Views 11 names, core Identity columns 4, tools Tools 20, researcher-guide table 11, all identical. The test pins those lists. + +A second test covers the failure this exists to prevent: a section of hyphenated names parses to all of them, where the old pattern yielded none and returned no error. + +--- + +## Guard the report-type vocabularies + +**Summary**: Gives each vocabulary one readable definition and holds the sentence that states it +against the right one. Lands before the core edit, so the core edit is what turns the guards green +rather than the guards being written to fit whatever the core happens to say. + +**Files affected**: +- `internal/dataset/reporttype.go`: the union vocabulary as a slice with `IsAllowedReportType` + reading it, and an accessor for the run vocabulary +- `internal/guidance/guard_test.go`: the two guards and their exemption lists + +**Estimated diff size**: ~120 lines + +There are two vocabularies and they are not the same set, which is the trap this step exists to +close. Measured: + +``` +run report_type (what the server sends on a run): [answers log usage] + null for Portal +union report_type (IsAllowedReportType, downloads): [answers usage log portal recovered] +``` + +The run set is `slugToType`'s values; `portal` and `recovered` are never on a run, they are +assigned locally to a download by `internal/fetch/report.go:162` and by reindex. A single guard +over `AllowedReportTypes()` would therefore force `portal` into a sentence about runs, which the +wire contradicts. + +```go +// AllowedReportTypes is the reports-union allowlist, in one place so the guidance +// guard and the predicate cannot disagree about what the vocabulary is. +func AllowedReportTypes() []string + +// RunReportTypes is what the server sends as a run's report_type. A Portal run +// sends none, which is why execution and not this list identifies one. +func RunReportTypes() []string +``` + +Neither guard can use `ParseCatalog`, since both sentences open with prose rather than a backticked +identifier. Neither needs to: matching `` `report_type` (...) `` against the rendered core returns +the list from the sentence as written. Verified against the current file, which yields `answers`, +`log`, `usage`, so the run guard is readable before the core edit and fails for the right reason. + +Both guards run both directions, as the view and tool guards do: every value the sentence names is +in its code list, and every value in the code list is named in its sentence or carries a one-line +exemption in the test. Both exemption sets ship empty, since this story states both vocabularies in +full. They exist so the next value added has to make a decision rather than be forgotten, and so +the decision is recorded beside the guard rather than in a commit message. + +--- + +## Teach the core the Portal report family, the slugs and the recipe + +**Summary**: The substance. One file, one commit, because the family, the slugs and the recipe are a single argument and splitting them would leave the core briefly incoherent. + +**Files affected**: +- `internal/guidance/src/core.md` +- `internal/guidance/guard_test.go` — the slug guard +- `internal/duck/views.go` and `internal/dataset/reporttype.go` — accessors for the two slug + inventories, both of which are unexported today (`dimensionViews[].slug` and `slugToType`'s keys), + so the guard can read either + +**Estimated diff size**: ~180 lines + +Four edits to `core.md`: + +**The run sentence keeps its list and gains the Portal clause** (`core.md:24`). The list is correct for runs and stays as it is; what it lacks is that a Portal run carries no `report_type` and is identified by `execution`. Without that, a model told runs have a `report_type` reaches for it on a Portal run and finds null. + +**The `downloads` entry states the download vocabulary** (`core.md:154`), which is where `portal` and `recovered` belong: both are assigned locally rather than arriving on a run. `recovered` carries the clause that makes it actionable, that a reindex assigns it to a CSV it cannot classify and re-fetching the run restores the real type, which pairs the value with the remedy `RECOVERED_PROVENANCE` already names. Together these two edits turn both guards from the previous step green. + +**The Portal/Athena family and the live-versus-snapshot rule**, in "Runs and their data", phrased as a decision rule because that is what the model needs it for: + +> Reports come in two families. **Athena** reports are computed in the background from the log archive: a run has a query state and its result never changes once it succeeds, so a fresh snapshot means duplicating the run. **Portal** reports (`report_type` `portal`) are computed from the Portal database on every request, so they list as `live` and a fresh read means re-pulling the same run, not duplicating it. Duplicating a Portal run is refused unless forced. + +**A `## Report slugs` section**, which is the guarded catalog and the thing that makes `reports_create` usable from the guidance alone: + +```markdown +## Report slugs + +A run is created from a report's slug. These are the ones a data pull starts from. The Portal also +offers aggregate metrics reports that are not listed here; their slugs come from an existing run or +from the researcher guide. + +- `student-id-mapping` — the learners' portal ids and the key that joins them to stored + records, with no names. A run of it is a valid run id for fetching answers, history and + attachments. +- `student-metadata` — the same learners with names and roster labels, joined on `learner_id`. +- `student-answers`, `student-assignment-usage` — per-student Athena reports. +- `student-actions`, `student-actions-with-metadata`, `teacher-actions` — Athena clickstream logs. +``` + +**The recipe**, as a numbered sequence. Verified to parse and to pass the no-commands guard: + +```markdown +### Pulling a cohort's work without authoring an Athena report + +1. Create a `student-id-mapping` run over the learners of interest, assembling the filter + from the available filter options. +2. Fetch that run's answers, history and attachments by its run id. +3. Fetch the run's own report CSV, which becomes `student_id_mapping`. +4. Create and fetch a `student-metadata` run over the same learners, which becomes + `student_metadata`. +5. Materialize the dataset before querying it when Materializing a dataset says it is worth it. +6. Query: `answers` joins `student_id_mapping` on `run_remote_endpoint = remote_endpoint`, + and `student_id_mapping` joins `student_metadata` on `learner_id`. See the + `student_id_mapping` entry for what a NULL join key means before filtering on it. + +Re-read any Portal run later by re-pulling the same run id; do not duplicate it. +``` + +The step wording is a constraint, not a style choice: naming the operation rather than the command is what keeps `TestCoreNamesNoCommand` green, and that belongs in a comment beside the guard rather than as folklore. + +Step 5 is a pointer to REPORT-113's rule, carrying the ordering only and no size condition, so there is one place that decides what "large" means. Stage 4 corrected it twice against the landed prose: the drafted wording opened "If the pull is large", which is exactly the condition it was meant to delegate, and it pointed at "when to materialize", which is not what the section is called. REPORT-113 titled it `## Materializing a dataset` (`core.md:189`), and the rule is the bullet beginning "It is worth doing once a dataset is large". Naming the operation rather than the command is still what keeps `TestCoreNamesNoCommand` green. + +Step 6 names the joins because they are the recipe's payoff, but it does not restate the withheld-key rule that governs them. That rule lives on the `student_id_mapping` entry (`core.md:132-137`): a NULL `run_remote_endpoint` is a learner with no secure key, and every such learner carries the same endpoint string, so the key is withheld rather than attributing one learner's answers to all of them. Restating it would put a correctness rule in two places with nothing holding them together; omitting the pointer would let a model follow the recipe and never meet it. Same treatment as the materialize step, and for the same reason. + +The two dimension-view entries gain their two missing facts and nothing else: the slug, and that a run of it drives the `get_*` verbs. Their existing dedup, withheld-key and `hide_names` prose is left untouched. + +**The slug guard**, following the three guards already in the file: + +```go +func TestGuidanceDocumentsOnlyRealSlugs(t *testing.T) { + documented, err := guidance.ParseCatalog(guidance.Core(), "Report slugs") + // every documented slug must exist in code; the reverse is deliberately not checked, + // because the portal offers aggregate reports that have no Go constant today +} +``` + +One direction only, for the reason the requirements give. It needs the union of `dataset.slugToType`'s keys and the dimension views' slugs, neither of which is currently exported, so each gets a small accessor rather than the guard reaching into package internals or a third copy of the list appearing in a test. + +The other half of what the guard means goes beside `slugToType` rather than in the test, because +that is the map that can be wrong and the file a future reader edits when adding a slug. The guard +carries a one-line pointer to it: + +```go +// slugToType is cc-data's own copy of the Athena slugs, not a roster of what the server offers. +// Nothing reconciles it: an unrecognized slug degrades with "unknown to this cc-data version" +// rather than failing, and the guidance guard can only prove the guidance matches this map, never +// that this map matches the server. REPORT-130 replaces the copy with a catalog read at runtime. +``` + +Verified that the union covers the documented set exactly: seven slugs in code, seven documented, none documented that code does not know. So the guard passes on the prose this plan writes, rather than being written and then having the prose trimmed to satisfy it. + +--- + +## Name the re-pull mechanism on each surface + +**Summary**: The per-surface half, which is where the two surfaces are allowed to differ and therefore where a test has to hold them together. + +**Files affected**: +- `internal/guidance/src/skill_header.md` — `--refresh` under "Fetching data" +- `internal/guidance/src/tools.md` — `refresh` on the `get_report` entry, so the MCP guidance carries it +- `internal/mcpserver/tools.go` — six tool descriptions +- `internal/guidance/guard_test.go` — the both-surfaces assertion +- `internal/mcpserver/server_test.go` — the description assertions + +**Estimated diff size**: ~120 lines + +`skill_header.md` gains `--refresh` on the `get report` line, since the core states the rule and cannot state the flag. + +`tools.md`'s `get_report` entry gains `refresh` too, and this is not redundant with the tool description. A tool's `Description` is registered with the MCP server and is **not** part of `guidance.Instructions()`, which renders `mcp_header.md` + `core.md` + `tools.md` only. Verified: none of three distinctive description strings appears in the rendered instructions. So the MCP surface's *guidance* learns about `refresh` only if `tools.md` says so, and a both-surfaces test that looked for it in `Instructions()` without this edit would fail on the day it was written. + +Six MCP descriptions gain one named fact each, rather than a general instruction to mention Portal runs: + +| Tool | Fact | +| --- | --- | +| `reports_list` | a run's execution tells an Athena run from a Portal one | +| `reports_filter_options` | it is how a Student ID Mapping run's filter is assembled | +| `get_report` | a Portal report is re-read by passing `refresh`, not by duplicating | +| `get_answers`, `get_history`, `get_attachments` | a Student ID Mapping run id is a valid source | + +Each is asserted in `server_test.go`. These are substring assertions on shipped strings, so the mutation they catch is real: delete the sentence and the test goes red. The drift guard cannot do this job, because it compares tool names and never reads a description. + +The both-surfaces assertion follows `TestBothSurfacesCarryTheAuthRemedy` exactly, and for the same reason: `refresh` is worded differently on each surface, so no name-comparison guard can notice one of them losing it. It compares `Skill()` against `Instructions()`, which is why the `tools.md` edit above is a precondition rather than a nicety. + +It is written only for `refresh`, deliberately. Asserting the core's content on both surfaces would be true by construction and is the decorative test to avoid. + +--- + +## Reconcile the researcher guide + +**Summary**: Last, because it can only be done once the guidance is final. Its expected outcome is a small diff or none. + +**Files affected**: +- `docs/researcher-guide.md` + +**Estimated diff size**: ~40 lines, possibly zero + +REPORT-94 already gave the guide a full Portal-reports treatment (`docs/researcher-guide.md:346-431`), so this is a consistency pass, not a rewrite. Read the finished core against the guide and fix only contradictions or omissions. The known candidate is section 4's "Make a run without the web form", which does not frame the mapping workflow as the way to pull a cohort. + +This step also reconciles REPORT-112's `logs` entry with the two-families framing, which is a core edit rather than a guide one but belongs with the other reconciliation work: the entry names three Athena slugs inline and needs to sit under the family the core now defines. + +The acceptance criterion is that the three documents agree, not that the guide changed. A pass that finds nothing is a passing outcome and should be recorded as one in the PR, rather than becoming a reason to edit prose that is already correct. + +Note the guide's view table is itself guarded (`TestResearcherGuideDocumentsEveryStaticView`), so any view-table edit here is already held by a shipped test. + +--- + +## Open Questions + +None. + +## Self-Review + +Roles: the engineer writing these tests, and the engineer reviewing the commits. Each finding was checked by building the proposed thing far enough to see whether the claim survived. + +### Test author + +#### RESOLVED: The both-surfaces test would have failed on the day it was written + +The plan asserted `refresh` on both surfaces following the auth-remedy precedent, which compares `guidance.Skill()` against `guidance.Instructions()`. But `refresh` was planned to land in `skill_header.md` on one side and in an **MCP tool description** on the other, and a tool `Description` is registered with the server, not rendered into the instructions. Verified: `Instructions()` contains none of three distinctive description strings, and `tools.md`'s `get_report` entry does not mention refresh today. + +So the test would have gone red immediately, and the tempting fix is the wrong one: weakening it to read the registered tools instead would make it pass while no longer holding the thing it exists to hold, which is that the *guidance* on both surfaces carries the rule. Fixed by adding `refresh` to `tools.md`'s `get_report` entry, which is rendered, and keeping the tool description as the separate, separately-asserted surface. + +This is the same class of mistake the requirements-stage review caught in the other direction: assuming a guard reads something it does not. + +#### Checked and not a problem: the slug guard passes on the prose this plan writes + +A guard written against prose that then has to be trimmed to satisfy it is a guard that has been fitted to the answer. Built the union the guard would use: seven slugs in code (five in `dataset.slugToType`, two in `dimensionViews`), seven documented by this plan, and nothing documented that the code does not know. Recorded so the next person does not re-derive it. + +### Commit reviewer + +#### RESOLVED: The section heading and the recipe subheading interact, and the plan did not say so + +`ParseCatalog` closes a section at the next heading of any level, so the `### Pulling a cohort's work` subheading ends the `## Report slugs` section. That is harmless as written, because the slug bullets precede it and the recipe's numbered items would not match the entry pattern anyway. It is a constraint on ordering, though: moving the recipe above the bullets would silently empty the guarded catalog, and `ParseCatalog` only errors when a section documents *no* names, so a partial reorder could shrink it without failing. + +Verified the current arrangement parses all seven slugs with the recipe subheading in place. The plan now carries the ordering constraint next to the section rather than leaving it to be rediscovered. diff --git a/specs/REPORT-95-portal-reports-guidance/requirements.md b/specs/REPORT-95-portal-reports-guidance/requirements.md new file mode 100644 index 0000000..7b00ed4 --- /dev/null +++ b/specs/REPORT-95-portal-reports-guidance/requirements.md @@ -0,0 +1,249 @@ +# Integrate Portal reports into the Claude skill and MCP guidance + +**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-95 +**Repo**: https://github.com/concord-consortium/cc-data-cli +**Implementation Spec**: [implementation.md](implementation.md) +**Status**: **In Development** + +## Overview + +Teach Claude the Portal-report workflow in the shared guidance both surfaces render, so that a researcher's data question reaches for the right report kind and the whole pull can be driven end to end. Today the guidance documents the two Portal-fed views in detail but never names the report family, never gives the slugs needed to create a run, and never mentions the flag that re-reads a live report. + +## Project Owner Overview + +cc-data can now create Portal report runs, download them, and join them to student answers. Claude cannot reliably drive any of that, because the guidance it reads was written for the Athena-only world and has only been patched where individual stories touched it. The result is a tool whose capabilities have outrun its instructions: a researcher asking Claude for class-level results gets steered down the Athena path, which is slower, needs a report authored first, and returns a frozen snapshot. + +This story closes that gap in the single shared source both the Claude Code skill and the MCP server render, so the two surfaces cannot drift apart on it. + +## Background + +REPORT-104 made one source of guidance rendered into two surfaces (`internal/guidance/guidance.go:27`, `:32`). The split matters here and is the root of several gaps below: + +- `Skill()` renders `skill_header.md` + `core.md`. +- `Instructions()` renders `mcp_header.md` + `core.md` + `tools.md`. + +So anything written in `tools.md` reaches the MCP server only, and the Claude Code skill never sees it. The drift guard compares *names* (views, tools, identity columns) in both directions but cannot notice a concept that reached one surface and not the other. + +**The ticket's background is out of date, and the spec is written against the code instead.** REPORT-95's description says the researcher guide "asserts the opposite in two places" and that both must be reversed. Both strings were removed by REPORT-94 in `564da81`, which replaced them with a full Portal-reports treatment: the Athena/Portal split with `execution` `async` vs `sync`, the `live` state, `get report --refresh`, both Portal report slugs, the `learner_id` join, the hide-names warning, and the one-row-per-learner dedup property (`docs/researcher-guide.md:346-431`). The same story landed the two dimension views and their guidance, which the ticket calls "minimal stubs"; they are not stubs, they are the longest entries in the views catalog. + +That does not leave this story with nothing. It relocates the work: the researcher guide is in good shape and the *guidance* is where the holes are. + +## Verified gaps + +Measured by rendering each surface and probing it, rather than by reading the sources: + +| Fact Claude needs | Skill | MCP instructions | +| --- | --- | --- | +| The phrase "Portal report" | **absent** | present | +| `student-id-mapping` slug | **absent** | **absent** | +| `student-metadata` slug | **absent** | **absent** | +| `--refresh` | **absent** | **absent** | +| `student_id_mapping` / `student_metadata` views | present | present | +| Re-pull rather than duplicate | present | present | + +Four specific defects follow: + +- **The core never says how a Portal run is recognized.** `core.md:24` reads "Report runs have a `report_type` (`answers`, `log`, `usage`)", and for a *run* that list is complete: it is exactly `slugToType`'s value set. A Portal run carries no `report_type` at all, which the wire pins (`portalRunWire` has `"report_type":null`, asserted in `internal/api/portal_download_test.go:52`) and the API type states as design: "report_type is null for a Portal run by design" (`internal/api/types.go:33`). The discriminator is `execution`. So the gap is not a missing enum value, it is that a model told runs have a `report_type` will reach for it on a Portal run, find null, and have nothing to fall back on. `portal` is a value cc-data synthesizes locally from `execution` (`internal/fetch/report.go:162`) and stores on the *download*, which is a different vocabulary in a different place. +- **Neither surface names a report slug.** `reports_create` takes a slug, so the first step of the documented workflow is unexecutable from the guidance alone: Claude has no way to know the string is `student-id-mapping`. The slugs exist in the researcher guide, which the model does not read. +- **Neither surface mentions `--refresh`.** The guidance tells the model to re-read a Portal report rather than duplicating it, and never says how. The flag exists (`cmd/get_report.go:71`), the fetch layer's own error message explains it (`internal/fetch/report.go:144`), and the MCP `get_report` tool already accepts the parameter, but its description is the bare sentence "Download a report CSV into a dataset." +- **The Portal/Athena distinction is MCP-only.** It lives on the `reports_duplicate` entry in `tools.md`, so the skill surface, which is the CLI-driving one, never learns it. + +The MCP descriptions for `reports_list`, `get_report`, `get_answers`, `get_history` and `get_attachments` say nothing about Portal runs or about a Student ID Mapping run being a valid source for the `get_*` verbs. + +## Requirements + +- The shared core names the Portal report family and the Athena/Portal distinction, so both surfaces carry it rather than the MCP surface alone. +- The run sentence at `core.md:24` keeps its list, which is correct for runs, and gains the fact it lacks: a Portal run has no `report_type`, and `execution` is what identifies it. Adding `portal` to that list instead would tell the model a Portal run carries `report_type: portal`, which the wire contradicts, and would undercut the `reports_list` description this story writes to teach exactly the opposite. +- The core carries the live-versus-snapshot model as a decision rule, not a fact: a Portal report is computed per request and is refreshed by re-pulling the same run; an Athena report is a frozen artifact and is refreshed by duplicating it into a new run. +- The guidance names every report slug the code knows: `student-id-mapping` and `student-metadata`, plus the five Athena slugs, so `reports_create` is usable from the guidance alone for any of them. Documenting only the two Portal slugs would leave the guard covering a fraction of the vocabulary while the other five stayed reachable but undocumented. +- The catalog says it is not exhaustive, in one sentence, because it is not. The Portal offers five aggregate metrics reports (`docs/researcher-guide.md:463-468`) whose slugs are in no Go inventory, and REPORT-128 merged three days ago specifically so two of them could be downloaded at all. cc-data never validates a slug before sending it, so they are fully usable and merely undocumented on the surface Claude reads, and there is no discovery path: `ListReports` returns the user's own runs, not the reports the server offers. Without that sentence a model reads seven slugs as the complete set and tells a researcher asking for school metrics that no such report exists. +- What the slug guard proves, and what it cannot, is stated where each half can be acted on. The guard carries its own rule: every documented slug must exist in code, the reverse is not checked, and why. The warning that cc-data's own inventory can be stale sits beside `slugToType`, because that is the map that goes stale and the file someone edits when adding a slug, and the guard points at it. The guard holds the guidance against cc-data's inventories, which catches a typo, since a wrong slug fails at the server with an error that says nothing about spelling. It cannot prove those inventories match the server's: `slugToType` is a static map nothing reconciles, and cc-data already expects it to go stale, warning "report slug %q is unknown to this cc-data version" on an unrecognized one. Green CI must not be read as evidence the slugs are current. +- The recipe names the joins but does not restate the rules that govern them. The withheld-join-key rule already lives on the `student_id_mapping` entry, and a correctness rule stated twice in one file with nothing holding the copies together is how the two drift. The recipe points at it instead, the same treatment the materialize step gets. +- The end-to-end recipe is documented: create a Student ID Mapping run, pull answers, history and attachments by its run id, download the Student ID Mapping and Student Metadata CSVs, materialize first if the pull is large, then query, joining answers and history on `remote_endpoint` and Student Metadata on `learner_id`. +- The recipe's materialize step is a **conditional pointer**, not a restatement: REPORT-113 owns when materializing is worth it, and this step references that rule rather than repeating the condition. The step exists because REPORT-115 templates its CLUE workflow on this recipe and puts materialize in exactly this position ("materialize (REPORT-113) when the history store is large"), for a corpus where it is closest to mandatory. A recipe with no slot for it would force 115 to invent one and the two workflows to diverge structurally. +- The re-pull mechanism is named on both surfaces: `--refresh` for the CLI, the `refresh` parameter for the MCP tool. +- The `student_id_mapping` and `student_metadata` view entries gain exactly the two facts they lack and are otherwise left alone: the slug a run of each is created from, and that such a run's id is a valid source for fetching answers, history and attachments. They already carry the dedup rule, the withheld-join-key rule and the `hide_names` rule. +- The MCP descriptions carry named facts rather than a general instruction to mention Portal runs, and a test asserts each one, since the drift guard checks tool names and never looks at description text: `reports_list` says a run's execution tells Athena from Portal; `reports_filter_options` says it is how a Student ID Mapping run's filter is assembled, since the recipe's first step sends the model straight to it; `get_report` says a Portal report is re-read with `refresh` rather than duplicated; and `get_answers`, `get_history` and `get_attachments` each say a Student ID Mapping run id is a valid source. +- A guard holds that every report slug named in the guidance exists in the code's slug inventories, so a typo cannot ship a slug that fails at the server. +- Each `report_type` vocabulary the guidance states is guarded against the code list that defines it, in both directions. There are two, and conflating them is what made the original requirement wrong: the run sentence is guarded against `slugToType`'s value set, which is what the server sends on a run; the `downloads` entry's vocabulary is guarded against `AllowedReportTypes()`, which is the reports-union allowlist. Every value is named in its own sentence or carries an explicit exemption with a reason. This story exists because guidance drifted from code, and correcting a list by hand would leave the mechanism intact, next to five guards that already do this for views, tools, identity columns, the auth remedy and command spellings. +- The `downloads` view entry states the download vocabulary its `report_type` column carries, which is where `portal` and `recovered` belong: both are values cc-data assigns locally, never values a run arrives with. `recovered` is documented rather than exempted because a reader meets it, on a download: an unclassifiable CSV comes back from a reindex as `"report_type": "recovered"` in `dataset show --json`, as `recovered` in the table's STATUS column, and beside the `RECOVERED_PROVENANCE` warning. It is stated with what produces it and that re-fetching restores the real type, so the value and its remedy arrive together. The core already alludes to such downloads without naming the value (`core.md:151`). +- Each vocabulary gets one readable definition in code, the way the view list already has one. `IsAllowedReportType` is a `switch`, so there is nothing a guard can read; it becomes a slice the switch consults, which is the same move `StaticViewNames()` made and the same reason. The run vocabulary needs no new list, since it is `slugToType`'s value set, but it does need an accessor, as the map is unexported. +- `ParseCatalog`'s identifier pattern is widened to accept hyphens, without which a slug catalog parses as empty and its guard silently checks nothing. The views, tools and identity-column catalogs share that pattern, so a test holds that all three still parse to the same names. +- The `--refresh` spelling is asserted on both surfaces by a test, following the auth remedy, which is the existing precedent for a rule that exists on both surfaces worded differently and that therefore no name-comparison guard can notice. Content placed in `core.md` needs no such test: both surfaces render the core by construction. +- REPORT-112's `logs` view entry is reconciled with the two-families framing this story introduces. It names `student-actions`, `student-actions-with-metadata` and `teacher-actions` inline, which are Athena slugs, so once the core says reports come in two families the entry has to sit visibly under one. This story owns it: it is the capstone that documents the finished surface, and REPORT-112 is already in review. +- The researcher guide is checked against the finished guidance and updated only where it is now wrong or silent. It is not rewritten: REPORT-94 already reversed the two assertions the ticket names. + +## Precondition + +This spec is written ahead of two of its inputs. REPORT-112 adds a `logs` entry to `core.md` and REPORT-113 adds the materialize prose to the same file, so **the branch is rebased and stage 4 re-run before implementation starts**, not after a conflict is discovered. The byte measurements below are pinned to the commit they were taken on and will move; the decision they support does not, since the recipe's cost is absolute and the core only grows. + +Verified against REPORT-112's branch already: with its `logs` entry in place, the widened catalog pattern parses the Views section to the same 12 names before and after, and no hyphenated slug leaks in from the entry's inline `student-actions` references. + +## Technical Notes + +- Command spellings cannot go in the core: `TestCoreNamesNoCommand` rejects any "`cc-data `" string there (`internal/guidance/guard_test.go`). So the *rule* ("refresh a Portal report by re-pulling the same run") goes in the core and the `--refresh` spelling goes in `skill_header.md`, with the MCP surface carrying the parameter on the tool description. This is the same split the auth remedy already uses. +- Two code-side slug inventories exist and do not overlap: `dataset.slugToType` holds the five Athena slugs (`internal/dataset/reporttype.go:16`), and `duck.dimensionViews[].slug` holds `student-id-mapping` and `student-metadata` (`internal/duck/views.go:494`, `:508`). A guard would union them. +- The aggregate Portal metrics reports (Summary Metrics by Assignment and the rest) have slugs that appear in no Go inventory, so a bidirectional slug guard would fail on them. See the open question. The deeper version of the same problem is that the server owns which reports exist and every local copy is a cache with no invalidation, which is why the repo already refused a slug map once: deriving a report's type from `execution` rather than a slug lookup is what lets "a Portal report added to the server later be recognized without a cc-data release" (`internal/fetch/report.go:158-160`). A server-side report catalog that the guidance points at instead of enumerating is the end state; it is filed as REPORT-130 and is out of scope here. +- `ParseCatalog` reads a named section and pulls the backticked identifiers that open each bullet or table row (`internal/guidance/catalog.go:13`), which is the shape any new guarded section has to take. +- The `report_type` guard cannot use `ParseCatalog`, because the sentence that carries the vocabulary opens with prose rather than with a backticked identifier. It does not need to: matching `` `report_type` (...) `` against the rendered core extracts the list from the sentence exactly as written. Verified by running it, which returned `answers`, `log`, `usage` from the current file. So the guard costs no restructuring and no bytes, which matters in a file that has grown 39% since this spec was drafted. +- The rendered skill is currently 12,464 bytes and the MCP instructions 13,462. Both are read on every session, and REPORT-89 is separately trying to protect the model's context budget. + +### Verified: the regex widening is safe, and the guarded section parses + +The slug guard turns on a change to a regex three existing catalogs share, so the claim that widening it changes nothing was run rather than argued. Parsed every catalog before and after widening `` `[a-z0-9_]+` `` to `` `[a-z0-9_-]+` ``: + +| Catalog | Before | After | +| --- | --- | --- | +| core, Views | 11 names | identical | +| core, Identity columns | 4 names | identical | +| tools, Tools | 20 names | identical | +| researcher guide, view table | 11 names | identical | + +The full guidance suite passes with the widening in place. A draft `## Report slugs` section then parsed all seven slugs (`student-id-mapping`, `student-metadata`, `student-answers`, `student-assignment-usage`, `student-actions`, `student-actions-with-metadata`, `teacher-actions`), where before the widening it would have yielded none of the hyphenated ones and reported success anyway. + +### Verified: the proposed core prose survives the no-commands guard + +The recipe is the first procedural content proposed for `core.md`, and `TestCoreNamesNoCommand` rejects any "`cc-data `" spelling there. Drafted the Portal family, the slug catalog and the five-step recipe into the real file and ran the suite: `TestCoreNamesNoCommand` passes, because the steps name the operation ("create a `student-id-mapping` run", "fetch that run's answers") rather than the command. That phrasing is a constraint on the prose, not an accident of it, so the implementation spec should say so; a later edit that helpfully adds the command spelling will fail CI. + +## Out of Scope + +- **Writing the "when to materialize" rule.** REPORT-113's spec assigns that prose to the shared core and the CLI spelling to the skill header, as part of that story. This story references the rule from the recipe and must not restate the condition. +- **CLUE document guidance.** REPORT-115 owns that, and its description names this story's prose as its template. +- Rewriting the researcher guide's Portal-reports treatment, which REPORT-94 landed. +- Any change to the views themselves, or to what `reports_create` accepts. + +## Stage 4 re-run (2026-09-11, rebased onto REPORT-113) + +The branch is now stacked on `REPORT-113-dataset-materialization`, so both inputs the Precondition +names are present: REPORT-112's `logs` entry and REPORT-113's materialize prose are both in +`core.md`. Every assumption above that could have moved was re-run against that state rather than +re-reasoned. Throwaway code, not committed. + +**Holds: `core.md:24` still reads as quoted.** Both stories appended sections rather than editing +near the top, so the `report_type` line has not moved and still omits `portal`. + +**Holds, and is now verified against both inputs: widening `leadingNames` to accept hyphens changes +nothing that parses today.** Measured over the rendered surfaces: Views 12 before and after, Tools +21 before and after, Identity columns 4 before and after, identical name-for-name in each. The +earlier check had only REPORT-112's branch; Tools is 21 because REPORT-113's `dataset_materialize` +entry is now in the catalog, and it parses the same either way. + +**Moved: every byte measurement, by more than the recipe it was sizing.** Re-measured on this +branch: core 14,334 bytes, skill 16,906, MCP instructions 17,722, against the 10,292 / 12,464 / +13,462 the spec records. The core grew 39%. The decision the numbers support is unchanged and +better supported, since the recipe's +1,532 is 10.7% of the new core against 14.9% of the old, but +the figures in **Technical Notes** and in the recipe decision are pinned to a commit that is two +merges behind and must be re-measured before they are quoted anywhere. + +**Moved, and this one changes the work: the pointer in recipe step 5 has no target.** The step +reads "see when to materialize", which assumed REPORT-113 would leave a rule findable under that +name. It did not: the section is `## Materializing a dataset` (`core.md:189`) and the rule is a +bullet beginning "It is worth doing once a dataset is large and the same questions are being asked +repeatedly". Nothing in the rendered core contains the phrase "when to materialize", so the step as +drafted sends a reader to a heading that does not exist. Step 5 must name the section that does. + +**Moved: step 5 carries the size condition it was supposed to delegate.** The implementation spec +says the step is "deliberately phrased without the size condition so there is one place that +decides what 'large' means", but the drafted step opens "If the pull is large". REPORT-113's bullet +already owns that condition, so the step should carry the ordering only, that materializing comes +before querying, and leave "large" to the core. + +**RESOLVED, and it grew the story: the core's `report_type` list is short by two, not one, and +hand-fixing it would leave the drift mechanism in place.** `IsAllowedReportType` accepts five values (`internal/dataset/reporttype.go:35`): +`answers`, `usage`, `log`, `portal` and `recovered`. The requirement adds `portal` alone, which +leaves the stated vocabulary still not matching the accepted vocabulary, and the requirement's own +justification ("the vocabulary it states is the vocabulary the code accepts") argues for both. +`recovered` is the synthesized type a reindexed CSV with no provenance gets. Verified that a reader +meets it: an aggregate-shaped CSV that `recoverReportType` cannot classify comes back through a +real reindex as `"report_type": "recovered"` in `dataset show --json`, as `recovered` in the +table's STATUS column, and beside a `RECOVERED_PROVENANCE` warning. + +**Resolved by guarding the list rather than by choosing a value.** Correcting three to five by hand +answers today's question and leaves the next report type free to drift, in the one story whose +subject is guidance that drifted. So the vocabulary gets a bidirectional guard like views, tools +and identity columns already have, the code gets one definition of the vocabulary for the guard to +read, and the question becomes structural: a type is documented or it is exempted with a reason, +and neither can be skipped. `recovered` is then documented, because it is reader-facing; an +exemption would have been defensible only for a value nobody sees. + +## Open Questions + +### RESOLVED: Should the slug guard be bidirectional, and what happens to the aggregate reports? + +**Context**: A guard that every guidance-named slug exists in code would have caught the fact that no slug is documented at all. But the two code inventories cover only seven slugs, and the Portal aggregate reports (Summary Metrics by Assignment, Detailed Metrics by Assignment, Teacher Status, Detailed Metrics by School, Summary Metrics by Subject Area) exist in the researcher guide and in the server, with no Go constant anywhere. A guard demanding that every documented slug exist in code would fail on those the moment anyone documents them. + +**Options considered**: +- A) One direction only: every slug the guidance names must exist in a code inventory. Catches the typo that matters, since a wrong slug fails at the server, and stays silent about slugs the code has no opinion on. +- B) Bidirectional, with the aggregate slugs added to a Go inventory first so both sides can agree. More complete, and it makes the code the roster of known reports, but it adds a list that must track the server. +- C) No guard. The slugs are few and the researcher guide already lists them. + +**Decision**: A, one direction. A wrong slug fails at the server with an error that says nothing about spelling, and that is the failure worth catching; the code having no constant for a report the server offers is not a defect the guidance should be blocked on. B would make the Go inventory a roster of every report the server exposes, which is a second thing to keep in sync with a system that changes without us. + +**But the guard does not work as assumed, and the discovery changes the work.** `ParseCatalog`'s `leadingNames` regex matches `` `[a-z0-9_]+` `` (`internal/guidance/catalog.go:13`), which excludes hyphens. Ran it against a slug section: `student-id-mapping` and `student-metadata` were both **silently skipped**, and the call returned success because one underscored control entry was present. A guard built on it as-is would pass while checking nothing, which is the exact shape of a test that cannot fail. + +So the guard requires widening the character class to accept hyphens. That regex is shared by the views, tools and identity-column catalogs, so the change needs a test proving those three still parse to identical name lists. No existing documented name contains a hyphen, so the widening cannot change their results, but that is an argument for writing the test rather than for skipping it. + +### RESOLVED: How much of the recipe belongs in the core, given the context budget? + +**Context**: The core is rendered into every session on both surfaces, and is already 12.4 KB. A full end-to-end recipe with the joins spelled out is perhaps 400 to 600 bytes more, on top of the Portal family, the slugs and the live-versus-snapshot rule. REPORT-89 is separately concerned with what the model has to carry. The alternative is a compressed decision rule in the core plus the worked recipe in the researcher guide, which the human reads and the model does not. + +**Options considered**: +- A) Full recipe in the core. The model can execute the workflow without the human relaying steps, which is the story's stated point. +- B) Decision rule in the core (which report kind, and that a mapping run's id drives the `get_*` verbs), worked example in the researcher guide only. Smallest context cost; relies on the model composing the steps from the view entries it already has. +- C) Full recipe in the core now, and let REPORT-89's cold walk-through decide whether it earns its bytes once the whole surface is measurable. + +**Decision**: A, and the measurement is what decides it. Both versions were drafted and measured against the real file: the minimal decision rule is +765 bytes on a 10,292-byte core, the full recipe is +1,532, so **the recipe itself costs 767 bytes**, roughly 200 tokens, and takes the rendered skill from 12,464 to 13,996. + +That is not where a context budget is won or lost, and the story exists precisely so the model can execute the workflow rather than narrate it. B would save 767 bytes by relying on the model to compose the sequence from view entries that document the joins but never say a mapping run's id drives the `get_*` verbs, which is the one fact it cannot infer. + +A and C are compatible rather than alternatives: write it now, and REPORT-89's cold walk-through can trim it with the whole surface in view, which is a better place to judge it from than here. + +### RESOLVED: Does this story still own researcher-guide work? + +**Context**: The ticket's guide requirement was to reverse two assertions, which REPORT-94 already did, and the guide's Portal treatment is now the most complete of the three documents. The remaining candidates are small: it does not mention `reports create` in section 4's "Make a run without the web form" in terms of the mapping workflow, and it will need whatever the guidance decides about slugs to stay consistent. + +**Options considered**: +- A) Yes, narrowed to a consistency pass: after the guidance is written, check the guide against it and fix only contradictions or omissions, with the acceptance criterion being that the three documents agree. +- B) No. Drop the guide from this story and note in the ticket that REPORT-94 discharged it. + +**Decision**: A, narrowed to a consistency pass. The guide is the only one of the three documents a human reads end to end, and this story is about to add slugs and a workflow to the other two; leaving it out would let the three drift on their first change. The pass is cheap because the guide is already correct: it looks for contradictions and omissions against the finished guidance, and changes nothing else. + +The acceptance criterion is that the three documents agree, not that the guide was edited. A pass that finds nothing to change is a passing outcome, and it should be recorded as one rather than treated as a reason to edit something. + +## Self-Review + +Roles: Senior Engineer, QA Engineer, Technical Writer. Findings that did not survive a check against the code are not recorded. + +### Senior Engineer + +#### RESOLVED: "Enrich the stubs" named no actual gap + +The requirement inherited the ticket's framing that the two dimension-view entries are minimal stubs to be enriched, which is both wrong and unactionable: they are the longest entries in the views catalog. Read them against the workflow and the gap is exactly two facts, neither of which is inferable from what is there: the slug a run is created from, and that such a run's id is a valid source for the `get_*` verbs. Everything else the workflow needs, the joins included, is already written. + +Fixed by naming the two facts. An instruction to "enrich" would have invited a rewrite of prose that is already correct, which is how a documentation change becomes a merge conflict for no gain. + +#### RESOLVED: The byte baseline is measured on a commit two unlanded stories both change + +The spec records the rendered surfaces at 12,464 and 13,462 bytes and uses the difference between a minimal and a full recipe to decide the context question. Checked what is in flight: REPORT-112's branch adds **32 lines to `core.md`** for the logs view, and REPORT-113's spec adds the materialize prose to the same file. So the baseline is stale before this is implemented, and `core.md` is very likely to conflict on merge. + +The decision it supports does not move: 767 bytes stays 767 bytes whatever the denominator, and the ratio only shrinks as the core grows. But the number is now pinned to the commit it was measured on, and the stage-4 re-run when 112 and 113 land is recorded as expected work rather than a surprise, along with the conflict. + +### QA Engineer + +#### RESOLVED: "Mention Portal runs where relevant" is a requirement that cannot fail + +The drift guard compares tool *names* in both directions and never reads a description (`internal/guidance/guard_test.go`). So a requirement phrased as "the descriptions mention Portal runs where relevant" has no way to be checked and no way to be wrong: any description satisfies it under a generous reading, and no test would go red if every description were left untouched. + +Fixed by naming the specific fact each of the five descriptions must carry and asserting each with a test. That is a test with a mutation to catch: delete the sentence and it goes red. + +#### RESOLVED: The one rule that needs a both-surfaces test was not distinguished from the ones that do not + +The spec asked for `--refresh` to be named on both surfaces without saying how that would be held, and asked for the Portal family to be in the core in the same breath, as though both needed the same protection. They do not, and conflating them would have produced either a redundant test or a missing one. + +Content in `core.md` is rendered by both surfaces by construction, so a both-surfaces assertion on it is true no matter what and is exactly the decorative test to avoid. The `--refresh` spelling is the opposite case: it lives in `skill_header.md` on one surface and in a tool description on the other, worded differently, which is precisely the shape `TestBothSurfacesCarryTheAuthRemedy` exists for. That precedent is now cited as the pattern to follow. + +### Technical Writer + +#### RESOLVED: The story had no stated success condition for the document it does not change + +The researcher-guide requirement said the guide is "checked and updated only where it is now wrong or silent", which leaves a reviewer unable to tell a completed pass from a skipped one. The resolution now states that the acceptance criterion is the three documents agreeing, and that a pass finding nothing to change is a passing outcome to be recorded rather than a prompt to edit something. From e4bf616ee03cf1e69fbd0583b7655e44b117b21a Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 08:51:40 -0400 Subject: [PATCH 02/12] refactor: let a catalog name carry a hyphen [REPORT-95] leadingNames excluded hyphens, so a section of report slugs parsed to nothing and ParseCatalog reported success, which would have left a guard over it checking nothing at all. Every report slug carries a hyphen, so the slug catalog this story adds is unreachable without the change. The four shipped catalogs are unaffected, and that needs no new test: the view, tool, identity-column and researcher-guide guards already compare each catalog against its code-derived set in both directions, so a name the widened pattern gained or lost fails one of them. Measured either way, all four parse to the same names as before. What does need a test is the failure the widening exists to prevent, and it goes red against the old pattern. --- internal/guidance/catalog.go | 5 +++-- internal/guidance/catalog_test.go | 16 ++++++++++++++++ .../implementation.md | 2 +- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/internal/guidance/catalog.go b/internal/guidance/catalog.go index c81fa2a..196d0a2 100644 --- a/internal/guidance/catalog.go +++ b/internal/guidance/catalog.go @@ -9,8 +9,9 @@ import ( // leadingNames matches the backticked identifiers that open a catalog entry, in either // markup the guarded files use: a bulleted list item ("- `x`, `y` — ...") or a markdown -// table row ("| `x`, `y` | ..."). -var leadingNames = regexp.MustCompile("^(?:- |\\| )((?:`[a-z0-9_]+`(?:, )?)+)") +// table row ("| `x`, `y` | ..."). Hyphens are allowed because report slugs carry them; +// without that a slug catalog parses as empty and its guard passes while checking nothing. +var leadingNames = regexp.MustCompile("^(?:- |\\| )((?:`[a-z0-9_-]+`(?:, )?)+)") // ParseCatalog returns every name documented in the first section whose heading // contains heading. The section runs to the next heading of any level, and a later diff --git a/internal/guidance/catalog_test.go b/internal/guidance/catalog_test.go index d714b05..22fcbd3 100644 --- a/internal/guidance/catalog_test.go +++ b/internal/guidance/catalog_test.go @@ -98,3 +98,19 @@ func TestMissingNamesTheAbsentEntries(t *testing.T) { t.Fatalf("nothing missing should report nothing, got %v", got) } } + +// TestParseCatalogReadsHyphenatedNames covers the failure the widened pattern +// exists to prevent: before it, a section of report slugs parsed to nothing and +// ParseCatalog reported success, so a guard over it checked nothing at all. +func TestParseCatalogReadsHyphenatedNames(t *testing.T) { + body := "## Report slugs\n\n- `student-id-mapping` — the learners' portal ids\n" + + "- `student-answers`, `student-assignment-usage` — per-student Athena reports\n" + got, err := ParseCatalog(body, "Report slugs") + if err != nil { + t.Fatal(err) + } + want := []string{"student-id-mapping", "student-answers", "student-assignment-usage"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } +} diff --git a/specs/REPORT-95-portal-reports-guidance/implementation.md b/specs/REPORT-95-portal-reports-guidance/implementation.md index b0864eb..dfa21be 100644 --- a/specs/REPORT-95-portal-reports-guidance/implementation.md +++ b/specs/REPORT-95-portal-reports-guidance/implementation.md @@ -34,7 +34,7 @@ Both inputs have landed in this branch's base as of 2026-09-11: it is stacked on var leadingNames = regexp.MustCompile("^(?:- |\\| )((?:`[a-z0-9_-]+`(?:, )?)+)") ``` -The test that matters is not that the new pattern accepts a hyphen, which is true by inspection. It is that the four shipped catalogs parse to the *same* names as before, so this cannot quietly change what the existing guards check. Measured before and after: core Views 11 names, core Identity columns 4, tools Tools 20, researcher-guide table 11, all identical. The test pins those lists. +The property that matters is not that the new pattern accepts a hyphen, which is true by inspection. It is that the four shipped catalogs parse to the *same* names as before, so this cannot quietly change what the existing guards check. That property needs no new test: `TestGuidanceDocumentsEveryStaticView`, `TestGuidanceDocumentsEveryTool`, `TestGuidanceDocumentsEveryIdentityColumn` and `TestResearcherGuideDocumentsEveryStaticView` already compare each catalog against its code-derived set in both directions, so any name the widened pattern gained or lost fails one of them. A new test asserting the same comparison could not fail unless one of those four also failed. Measured before and after regardless: core Views 12 names, core Identity columns 4, tools Tools 21, researcher-guide table 12, all identical. A second test covers the failure this exists to prevent: a section of hyphenated names parses to all of them, where the old pattern yielded none and returned no error. From 06d231ffcd4eedc26626ae7b414ad552913530c4 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 08:54:33 -0400 Subject: [PATCH 03/12] feat: give each report_type vocabulary one readable definition [REPORT-95] There are two and they are not the same set. A run carries answers, log or usage, and nothing else: a Portal run's report_type is null on the wire, which is why execution identifies one. The reports union additionally admits portal and recovered, both assigned locally, the first derived from a run's execution and the second by a reindex that cannot classify a CSV. IsAllowedReportType was a switch, which offers a guard nothing to read, so the allowlist becomes a slice the predicate consults and each vocabulary gets an accessor. The run vocabulary is now guarded against the sentence that states it, both directions: a value the guidance names must be accepted by the code, and a value the code accepts must be named or carry an exemption with a reason. The exemption set ships empty and exists so a value added later has to make a decision rather than be forgotten. The guard reads the sentence as written, since it opens with prose rather than a backticked name and ParseCatalog cannot help. The download vocabulary's guard is not here. It was written first and confirmed to fail for the right reason, that the core states no such sentence yet, and it lands with the sentence rather than leaving a commit red. slugToType gains a comment saying what it is: cc-data's own copy of the Athena slugs, not a roster of what the server offers, and the thing the guidance guard cannot vouch for. --- internal/dataset/reporttype.go | 45 +++++++++++++++++++++++++---- internal/guidance/guard_test.go | 50 +++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/internal/dataset/reporttype.go b/internal/dataset/reporttype.go index 8a0f48b..8dcfd4e 100644 --- a/internal/dataset/reporttype.go +++ b/internal/dataset/reporttype.go @@ -1,5 +1,7 @@ package dataset +import "sort" + // Report type vocabulary. const ( ReportTypeAnswers = "answers" @@ -13,6 +15,11 @@ const ( ReportTypePortal = "portal" ) +// slugToType is cc-data's own copy of the Athena slugs, not a roster of what the server +// offers. Nothing reconciles it: an unrecognized slug degrades with "unknown to this +// cc-data version" rather than failing, and the guidance guard can only prove the guidance +// matches this map, never that this map matches the server. REPORT-130 replaces the copy +// with a catalog read at runtime. var slugToType = map[string]string{ "student-answers": ReportTypeAnswers, "student-assignment-usage": ReportTypeUsage, @@ -28,12 +35,40 @@ func ReportTypeFromSlug(slug string) (string, bool) { return t, ok } -// IsAllowedReportType reports whether a report type is in the reports-union -// allowlist (answers/usage/log/portal, plus the reindex-only recovered value). +// allowedReportTypes is the reports-union allowlist, in one place so the guidance guard +// and the predicate cannot disagree about what the vocabulary is. +var allowedReportTypes = []string{ + ReportTypeAnswers, ReportTypeUsage, ReportTypeLog, ReportTypePortal, ReportTypeRecovered, +} + +// AllowedReportTypes returns the report types the reports union admits. portal and +// recovered are assigned locally rather than arriving on a run: portal is derived from a +// run's execution, recovered from a reindex that cannot classify a CSV. +func AllowedReportTypes() []string { + return append([]string(nil), allowedReportTypes...) +} + +// RunReportTypes returns what the server sends as a run's report_type. A Portal run sends +// none, which is why execution and not this list identifies one. +func RunReportTypes() []string { + seen := map[string]bool{} + var out []string + for _, t := range slugToType { + if !seen[t] { + seen[t] = true + out = append(out, t) + } + } + sort.Strings(out) + return out +} + +// IsAllowedReportType reports whether a report type is in the reports-union allowlist. func IsAllowedReportType(t string) bool { - switch t { - case ReportTypeAnswers, ReportTypeUsage, ReportTypeLog, ReportTypePortal, ReportTypeRecovered: - return true + for _, a := range allowedReportTypes { + if t == a { + return true + } } return false } diff --git a/internal/guidance/guard_test.go b/internal/guidance/guard_test.go index 78eae6f..2b01723 100644 --- a/internal/guidance/guard_test.go +++ b/internal/guidance/guard_test.go @@ -3,9 +3,12 @@ package guidance_test import ( "context" "os" + "regexp" + "sort" "strings" "testing" + "github.com/concord-consortium/cc-data-cli/internal/dataset" "github.com/concord-consortium/cc-data-cli/internal/duck" "github.com/concord-consortium/cc-data-cli/internal/guidance" "github.com/concord-consortium/cc-data-cli/internal/mcpserver" @@ -140,3 +143,50 @@ func TestGuardDetectsAnUndocumentedName(t *testing.T) { t.Fatalf("guard did not report the missing name, got %v", m) } } + +// reportTypesIn reads the vocabulary a core sentence states. The sentences open +// with prose rather than a backticked name, so ParseCatalog cannot read them. +func reportTypesIn(t *testing.T, body, after string) []string { + t.Helper() + re := regexp.MustCompile(regexp.QuoteMeta(after) + " `report_type` \\(((?:`[a-z]+`(?:, )?)+)\\)") + m := re.FindStringSubmatch(body) + if m == nil { + t.Fatalf("no report_type vocabulary found after %q", after) + } + var out []string + for _, n := range regexp.MustCompile("`([a-z]+)`").FindAllStringSubmatch(m[1], -1) { + out = append(out, n[1]) + } + sort.Strings(out) + return out +} + +// runReportTypeExemptions records a value the code accepts that the guidance +// deliberately does not name, with the reason. It is empty: the guidance states the +// vocabulary in full. It exists so a value added later has to make a decision rather +// than be forgotten. +var runReportTypeExemptions = map[string]string{} + +func TestGuidanceStatesTheRunReportTypes(t *testing.T) { + assertVocabulary(t, reportTypesIn(t, guidance.Core(), "Report runs have a"), + dataset.RunReportTypes(), runReportTypeExemptions, "run") +} + +func assertVocabulary(t *testing.T, documented, inCode []string, exempt map[string]string, what string) { + t.Helper() + if len(documented) == 0 { + t.Fatalf("%s vocabulary parsed as empty, so this checks nothing", what) + } + if m := guidance.Missing(documented, inCode); len(m) > 0 { + t.Errorf("%s vocabulary names %v, which the code does not accept", what, m) + } + var undocumented []string + for _, v := range guidance.Missing(inCode, documented) { + if _, ok := exempt[v]; !ok { + undocumented = append(undocumented, v) + } + } + if len(undocumented) > 0 { + t.Errorf("code accepts %v, which the %s vocabulary neither names nor exempts", undocumented, what) + } +} From d16e548a2b3d76322b6555f1295c4ae6ef258821 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 08:58:01 -0400 Subject: [PATCH 04/12] feat: teach the core the Portal family, the slugs and the pull recipe [REPORT-95] The guidance was written for the Athena-only world. It never named the Portal report family, never gave a single report slug, and never said how to re-read a live report, so the first step of the documented workflow was unexecutable from the guidance alone and a researcher asking for class-level results got steered down the slower path. Five edits to the shared core, which both surfaces render by construction. The two families as a decision rule rather than a fact, since what a model needs is which one to reach for. The run sentence keeping its list and gaining what it lacked: a Portal run has no report_type, and execution identifies it. The downloads entry stating its own vocabulary, where portal and recovered belong, both being values cc-data assigns rather than values a run arrives with. A guarded slug catalog, which is what makes creating a run possible from the guidance alone, saying in one sentence that it is not exhaustive because the Portal offers aggregate reports no Go inventory knows. And the end-to-end recipe, from creating a mapping run to the final join. The recipe names operations rather than commands, which is what keeps the no-commands guard green, and it points at rules rather than restating them: at the materialize rule for when copying is worth it, and at the mapping view's entry for what a NULL join key means. A correctness rule stated twice in one file with nothing holding the copies together is how the two drift. Two guards close behind the prose. The download vocabulary now has the sentence its guard was written against, and every documented slug must exist in code. The slug guard runs one direction only, because the reverse would fail on reports the server offers and no Go constant names. --- internal/dataset/reporttype.go | 10 +++ internal/duck/views.go | 10 +++ internal/guidance/guard_test.go | 38 +++++++++--- internal/guidance/src/core.md | 62 ++++++++++++++++--- .../implementation.md | 4 +- 5 files changed, 107 insertions(+), 17 deletions(-) diff --git a/internal/dataset/reporttype.go b/internal/dataset/reporttype.go index 8dcfd4e..58d560c 100644 --- a/internal/dataset/reporttype.go +++ b/internal/dataset/reporttype.go @@ -72,3 +72,13 @@ func IsAllowedReportType(t string) bool { } return false } + +// ReportSlugs returns the Athena slugs cc-data knows. +func ReportSlugs() []string { + var out []string + for slug := range slugToType { + out = append(out, slug) + } + sort.Strings(out) + return out +} diff --git a/internal/duck/views.go b/internal/duck/views.go index 7140ce1..ca6d38a 100644 --- a/internal/duck/views.go +++ b/internal/duck/views.go @@ -929,3 +929,13 @@ func hideNamesLiteral(dl dataset.Download) string { func sqlTimestamp(t time.Time) string { return fmt.Sprintf("CAST(%s AS TIMESTAMP)", sqlStr(t.UTC().Format("2006-01-02 15:04:05.999999"))) } + +// DimensionSlugs returns the report slugs the dimension views are built from. +func DimensionSlugs() []string { + var out []string + for _, d := range dimensionViews { + out = append(out, d.slug) + } + sort.Strings(out) + return out +} diff --git a/internal/guidance/guard_test.go b/internal/guidance/guard_test.go index 2b01723..c177fd7 100644 --- a/internal/guidance/guard_test.go +++ b/internal/guidance/guard_test.go @@ -144,11 +144,12 @@ func TestGuardDetectsAnUndocumentedName(t *testing.T) { } } -// reportTypesIn reads the vocabulary a core sentence states. The sentences open -// with prose rather than a backticked name, so ParseCatalog cannot read them. +// reportTypesIn reads the vocabulary a core sentence states. The sentences open with +// prose rather than a backticked name, so ParseCatalog cannot read them, and the core +// is hard-wrapped, so the list can span lines. func reportTypesIn(t *testing.T, body, after string) []string { t.Helper() - re := regexp.MustCompile(regexp.QuoteMeta(after) + " `report_type` \\(((?:`[a-z]+`(?:, )?)+)\\)") + re := regexp.MustCompile(regexp.QuoteMeta(after) + "\\s+`report_type`\\s*\\(((?:`[a-z]+`(?:,\\s*)?)+)\\)") m := re.FindStringSubmatch(body) if m == nil { t.Fatalf("no report_type vocabulary found after %q", after) @@ -161,11 +162,13 @@ func reportTypesIn(t *testing.T, body, after string) []string { return out } -// runReportTypeExemptions records a value the code accepts that the guidance -// deliberately does not name, with the reason. It is empty: the guidance states the -// vocabulary in full. It exists so a value added later has to make a decision rather -// than be forgotten. -var runReportTypeExemptions = map[string]string{} +// These record a value the code accepts that the guidance deliberately does not name, +// with the reason. Both are empty: the guidance states both vocabularies in full. They +// exist so a value added later has to make a decision rather than be forgotten. +var ( + runReportTypeExemptions = map[string]string{} + downloadReportTypeExemptions = map[string]string{} +) func TestGuidanceStatesTheRunReportTypes(t *testing.T) { assertVocabulary(t, reportTypesIn(t, guidance.Core(), "Report runs have a"), @@ -190,3 +193,22 @@ func assertVocabulary(t *testing.T, documented, inCode []string, exempt map[stri t.Errorf("code accepts %v, which the %s vocabulary neither names nor exempts", undocumented, what) } } + +func TestGuidanceStatesTheDownloadReportTypes(t *testing.T) { + assertVocabulary(t, reportTypesIn(t, guidance.Core(), "A download's"), + dataset.AllowedReportTypes(), downloadReportTypeExemptions, "download") +} + +// TestGuidanceDocumentsOnlyRealSlugs runs one direction only. The reverse is +// deliberately not checked, because the portal offers aggregate reports that have +// no Go constant today. What this cannot prove is recorded beside slugToType. +func TestGuidanceDocumentsOnlyRealSlugs(t *testing.T) { + documented, err := guidance.ParseCatalog(guidance.Core(), "Report slugs") + if err != nil { + t.Fatal(err) + } + inCode := append(dataset.ReportSlugs(), duck.DimensionSlugs()...) + if m := guidance.Missing(documented, inCode); len(m) > 0 { + t.Fatalf("guidance names slugs the code does not know: %v", m) + } +} diff --git a/internal/guidance/src/core.md b/internal/guidance/src/core.md index 1e3244f..a8ce9c3 100644 --- a/internal/guidance/src/core.md +++ b/internal/guidance/src/core.md @@ -21,8 +21,15 @@ - Datasets are duplicate-free by construction: re-fetching a run replaces its records. Create a dataset per point-in-time pull to compare over time. -- Report runs have a `report_type` (`answers`, `log`, `usage`). Log runs (slug - `student-actions`) are fetchable as a report too and yield a clickstream +- Reports come in two families. **Athena** reports are computed in the background + from the log archive: a run has a query state and its result never changes once + it succeeds, so a fresh snapshot means duplicating the run. **Portal** reports + are computed from the Portal database on every request, so they list as `live` + and a fresh read means re-pulling the same run, not duplicating it. Duplicating + a Portal run is refused unless forced. +- Report runs have a `report_type` (`answers`, `log`, `usage`). A Portal run has + none: its `report_type` is null and `execution` is what identifies it. Log runs + (slug `student-actions`) are fetchable as a report too and yield a clickstream event log (columns include `session`, `application`, `activity`, `event`, `event_value`, `time`, `parameters`, `extras`, `run_remote_endpoint`, `timestamp`, `user_id`, `primary_user_id`): process, timing, and sequence data. @@ -129,8 +136,10 @@ like `wildfire_2026.answers`): `state`), not just the current-answer one, so you can diff every saved snapshot of a doc across a session's history. Binary attachments (audio, images) are excluded here (not UTF-8 text) but remain downloadable via `attachment_files`. -- `student_id_mapping` — one row per `learner_id` from Student ID Mapping runs, - deduplicated across runs with the latest fetch winning. Join to `answers` and +- `student_id_mapping` — one row per `learner_id` from Student ID Mapping runs + (slug `student-id-mapping`), deduplicated across runs with the latest fetch + winning. Such a run's id is a valid source for fetching answers, history and + attachments. Join to `answers` and `history` on `run_remote_endpoint = remote_endpoint`. A NULL `run_remote_endpoint` is a learner with no secure key, not missing data: every such learner carries the same endpoint string, so the join key is withheld @@ -140,9 +149,10 @@ like `wildfire_2026.answers`): report_`. Do not compare against this view's rows for that run: it deduplicates **across** runs, so a learner a later run also holds is absent here without the earlier run having repeated anything. -- `student_metadata` — one row per `learner_id` from Student Metadata runs, same - dedupe and the same `run_remote_endpoint` rule, carrying the names and roster - labels the mapping view deliberately has none of. Join to +- `student_metadata` — one row per `learner_id` from Student Metadata runs (slug + `student-metadata`), same dedupe and the same `run_remote_endpoint` rule, + carrying the names and roster labels the mapping view deliberately has none of. + Such a run's id is a valid source for fetching answers, history and attachments. Join to `student_id_mapping` on `learner_id`. `hide_names` is the run's own setting: where it is true, `student_name` holds the student id and `username` a hash, so a dataset holding runs fetched under different roles is filterable rather @@ -152,7 +162,11 @@ like `wildfire_2026.answers`): `reports`, which has no such column, so name-sensitive work belongs on this view or on a type-qualified `downloads` join. - `downloads` — a manifest dimension table: `run_id`, `type`, `slug`, - `report_type`, `hide_names` and `complete`. It is where a per-download fact + `report_type`, `hide_names` and `complete`. A download's `report_type` + (`answers`, `log`, `usage`, `portal`, `recovered`) is cc-data's own, not the + run's: `portal` is derived from a Portal run's execution, and `recovered` is + what a reindex assigns to a CSV it cannot classify, which re-fetching the run + replaces with the real type. It is where a per-download fact belongs, so it is the join for anything that varies by run rather than by row. **One row per download, not per run**: a run that had its report, answers and history pulled has three, so join it type-qualified (`AND d.type = 'report'`) @@ -168,6 +182,38 @@ to attach the person: `user_id` is the Portal user (the learner) and `learner_id is that user in one offering. Count distinct learners by `user_id` (or `learner_id`); cross-portal identity is out of scope. +## Report slugs + +A run is created from a report's slug. These are the ones a data pull starts +from. The Portal also offers aggregate metrics reports that are not listed here; +their slugs come from an existing run or from the researcher guide. + +- `student-id-mapping` — the learners' portal ids and the key that joins them to + stored records, with no names. A run of it is a valid run id for fetching + answers, history and attachments. +- `student-metadata` — the same learners with names and roster labels, joined on + `learner_id`. +- `student-answers`, `student-assignment-usage` — per-student Athena reports. +- `student-actions`, `student-actions-with-metadata`, `teacher-actions` — Athena + clickstream logs. + +### Pulling a cohort's work without authoring an Athena report + +1. Create a `student-id-mapping` run over the learners of interest, assembling + the filter from the available filter options. +2. Fetch that run's answers, history and attachments by its run id. +3. Fetch the run's own report CSV, which becomes `student_id_mapping`. +4. Create and fetch a `student-metadata` run over the same learners, which + becomes `student_metadata`. +5. Materialize the dataset before querying it when Materializing a dataset says + it is worth it. +6. Query: `answers` joins `student_id_mapping` on `run_remote_endpoint = + remote_endpoint`, and `student_id_mapping` joins `student_metadata` on + `learner_id`. See the `student_id_mapping` entry for what a NULL join key + means before filtering on it. + +Re-read any Portal run later by re-pulling the same run id; do not duplicate it. + ## Identity columns A record is identified across the stores by these columns, which are also the diff --git a/specs/REPORT-95-portal-reports-guidance/implementation.md b/specs/REPORT-95-portal-reports-guidance/implementation.md index dfa21be..1362bba 100644 --- a/specs/REPORT-95-portal-reports-guidance/implementation.md +++ b/specs/REPORT-95-portal-reports-guidance/implementation.md @@ -110,7 +110,9 @@ Four edits to `core.md`: **The Portal/Athena family and the live-versus-snapshot rule**, in "Runs and their data", phrased as a decision rule because that is what the model needs it for: -> Reports come in two families. **Athena** reports are computed in the background from the log archive: a run has a query state and its result never changes once it succeeds, so a fresh snapshot means duplicating the run. **Portal** reports (`report_type` `portal`) are computed from the Portal database on every request, so they list as `live` and a fresh read means re-pulling the same run, not duplicating it. Duplicating a Portal run is refused unless forced. +> Reports come in two families. **Athena** reports are computed in the background from the log archive: a run has a query state and its result never changes once it succeeds, so a fresh snapshot means duplicating the run. **Portal** reports are computed from the Portal database on every request, so they list as `live` and a fresh read means re-pulling the same run, not duplicating it. Duplicating a Portal run is refused unless forced. + +The parenthetical "(`report_type` `portal`)" an earlier draft carried is deliberately gone: a Portal run has no `report_type`, which the sentence above this one now says, and repeating the download-side value here would reintroduce the conflation. **A `## Report slugs` section**, which is the guarded catalog and the thing that makes `reports_create` usable from the guidance alone: From c0131500cf75c84f7ee819bbf02ed6987574fcfa Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 09:01:06 -0400 Subject: [PATCH 05/12] feat: name the re-pull mechanism on each surface [REPORT-95] The core states the rule, that a Portal report is refreshed by re-pulling the same run rather than duplicating it, and cannot state the flag, because the no-commands guard rejects any cc-data spelling there. So the skill header gains --refresh and the MCP guidance gains refresh, which is the split the auth remedy already uses. The tools.md edit is not redundant with the tool description. A tool's Description is registered with the MCP server and is not part of the rendered instructions, which are the MCP header plus the core plus tools.md, so the MCP surface's guidance learns about refresh only if tools.md says so. Six descriptions gain one named fact each rather than a general instruction to mention Portal runs: how to tell an Athena run from a Portal one, how a mapping run's filter is assembled, how a Portal report is re-read, and that a mapping run id is a valid source for each of the three bulk fetches. The drift guard cannot hold any of this, because it compares tool names and never reads a description, so each is asserted directly. The both-surfaces test covers only the per-surface halves. The rule itself lives in the core, which both surfaces render by construction, so asserting it on each would be true by construction and would test nothing. --- internal/guidance/guard_test.go | 14 ++++++++++++ internal/guidance/src/skill_header.md | 4 +++- internal/guidance/src/tools.md | 4 +++- internal/mcpserver/server_test.go | 31 +++++++++++++++++++++++++++ internal/mcpserver/tools.go | 12 +++++------ 5 files changed, 57 insertions(+), 8 deletions(-) diff --git a/internal/guidance/guard_test.go b/internal/guidance/guard_test.go index c177fd7..732f6dc 100644 --- a/internal/guidance/guard_test.go +++ b/internal/guidance/guard_test.go @@ -212,3 +212,17 @@ func TestGuidanceDocumentsOnlyRealSlugs(t *testing.T) { t.Fatalf("guidance names slugs the code does not know: %v", m) } } + +// The re-pull mechanism is the second piece of guidance that exists on both surfaces +// worded differently, the CLI spelling it --refresh and the MCP surface refresh, so no +// inventory comparison can notice one of them losing it. Only the per-surface halves are +// asserted: the rule itself is in the core, which both render by construction, so +// checking it here would be true by construction and would test nothing. +func TestBothSurfacesCarryTheRePullMechanism(t *testing.T) { + if !strings.Contains(guidance.Skill(), "--refresh") { + t.Error("skill: the --refresh spelling is missing") + } + if !strings.Contains(guidance.Instructions(), "passing `refresh`") { + t.Error("instructions: the refresh parameter is missing") + } +} diff --git a/internal/guidance/src/skill_header.md b/internal/guidance/src/skill_header.md index 8f8d019..b808689 100644 --- a/internal/guidance/src/skill_header.md +++ b/internal/guidance/src/skill_header.md @@ -40,7 +40,9 @@ guessing flags. ## Fetching data -- `cc-data get report --dataset ` — the report CSV. +- `cc-data get report --dataset ` — the report CSV. A Portal report + is computed per request, so re-read it with `--refresh` rather than duplicating + the run. - `cc-data get answers --dataset ` — student answers. - `cc-data get history --dataset ` — full interactive state history. - `cc-data get attachments --dataset ` — file attachments (requires diff --git a/internal/guidance/src/tools.md b/internal/guidance/src/tools.md index 418424a..8788f41 100644 --- a/internal/guidance/src/tools.md +++ b/internal/guidance/src/tools.md @@ -16,7 +16,9 @@ once they finish, so duplicating is how they are re-run; a Portal report is computed live, so re-read it with `get_report` instead and pass `force` only if a second run id is genuinely wanted. -- `get_report` — the report CSV for a run, into a dataset. +- `get_report` — the report CSV for a run, into a dataset. A Portal report is + computed per request, so re-read it by passing `refresh` rather than + duplicating the run. - `get_answers`, `get_history` — a run's student answers, and the full series of how each answer's interactive state evolved, into a dataset. - `get_attachments` — a run's file attachments, into a dataset. Fetch that run's diff --git a/internal/mcpserver/server_test.go b/internal/mcpserver/server_test.go index 6da790c..f37d813 100644 --- a/internal/mcpserver/server_test.go +++ b/internal/mcpserver/server_test.go @@ -714,3 +714,34 @@ func TestMCPReportsCreateOmitsAnAbsentFilter(t *testing.T) { t.Errorf("an absent report_filter must be omitted; body = %s", raw) } } + +// Each description carries one named fact that the drift guard cannot see, because it +// compares tool names and never reads description text. Substring assertions on shipped +// strings, so deleting a sentence turns one red. +func TestToolDescriptionsCarryTheirPortalFacts(t *testing.T) { + cs := connect(t) + res, err := cs.ListTools(context.Background(), nil) + if err != nil { + t.Fatal(err) + } + desc := map[string]string{} + for _, tl := range res.Tools { + desc[tl.Name] = tl.Description + } + for tool, fact := range map[string]string{ + "reports_list": "execution tells an Athena run from a Portal one", + "reports_filter_options": "Student ID Mapping run's filter is assembled", + "get_report": "re-read it by passing refresh", + "get_answers": "Student ID Mapping run id is a valid source", + "get_history": "Student ID Mapping run id is a valid source", + "get_attachments": "Student ID Mapping run id is a valid source", + } { + if _, ok := desc[tool]; !ok { + t.Errorf("%s is not registered", tool) + continue + } + if !strings.Contains(desc[tool], fact) { + t.Errorf("%s description lost %q", tool, fact) + } + } +} diff --git a/internal/mcpserver/tools.go b/internal/mcpserver/tools.go index d411303..70b7e41 100644 --- a/internal/mcpserver/tools.go +++ b/internal/mcpserver/tools.go @@ -36,7 +36,7 @@ func registerTools(s *mcp.Server, opts Options) { return nil, res, err }) - addTool(s, &mcp.Tool{Name: "reports_list", Description: "List the user's report runs for a portal. The portal may be a hostname or an environment alias (prod / staging / dev).", Annotations: readOnly}, + addTool(s, &mcp.Tool{Name: "reports_list", Description: "List the user's report runs for a portal. A run's execution tells an Athena run from a Portal one: async is Athena, sync is Portal. The portal may be a hostname or an environment alias (prod / staging / dev).", Annotations: readOnly}, func(ctx context.Context, req *mcp.CallToolRequest, in portalIn) (*mcp.CallToolResult, reportview.RunsPayload, error) { client, err := portalClient(in.Portal) if err != nil { @@ -49,7 +49,7 @@ func registerTools(s *mcp.Server, opts Options) { return nil, reportview.Runs(runs), nil }) - addTool(s, &mcp.Tool{Name: "reports_filter_options", Description: "List the values a report filter dimension offers the user, narrowed by any selections already made, so a filter can be assembled without the web form. Also answers \"what data can I see?\" on its own. Pass report_filter to narrow (the same object reports_list returns on a run), search to match labels, and report_slug to restrict to a report that offers the dimension. Set include_count=true when the user asks how many there are, and all=true to walk the pages, which stops after 1000 options and sets truncated with next_page_token to continue from. The portal may be a hostname or an environment alias (prod / staging / dev).", Annotations: readOnly}, + addTool(s, &mcp.Tool{Name: "reports_filter_options", Description: "List the values a report filter dimension offers the user, narrowed by any selections already made, so a filter can be assembled without the web form. Also answers \"what data can I see?\" on its own. Pass report_filter to narrow (the same object reports_list returns on a run), search to match labels, and report_slug to restrict to a report that offers the dimension. Set include_count=true when the user asks how many there are, and all=true to walk the pages, which stops after 1000 options and sets truncated with next_page_token to continue from. It is also how a Student ID Mapping run's filter is assembled, which is the first step of a cohort pull. The portal may be a hostname or an environment alias (prod / staging / dev).", Annotations: readOnly}, func(ctx context.Context, req *mcp.CallToolRequest, in reportsFilterOptionsIn) (*mcp.CallToolResult, reportview.FilterOptionsPayload, error) { client, err := portalClient(in.Portal) if err != nil { @@ -118,7 +118,7 @@ func registerTools(s *mcp.Server, opts Options) { return nil, reportview.RunPayload{Run: reportview.ToRunJSON(run)}, nil }) - addTool(s, &mcp.Tool{Name: "get_report", Description: "Download a report CSV into a dataset."}, + addTool(s, &mcp.Tool{Name: "get_report", Description: "Download a report CSV into a dataset. A Portal report is computed per request, so re-read it by passing refresh rather than duplicating the run."}, func(ctx context.Context, req *mcp.CallToolRequest, in getReportIn) (*mcp.CallToolResult, mapOut, error) { d, client, err := openForFetch(in.Dataset) if err != nil { @@ -131,12 +131,12 @@ func registerTools(s *mcp.Server, opts Options) { return fetchResult(fetch.FetchReport(ctx, o)) }) - addTool(s, &mcp.Tool{Name: "get_answers", Description: "Download a run's student answers into a dataset."}, + addTool(s, &mcp.Tool{Name: "get_answers", Description: "Download a run's student answers into a dataset. A Student ID Mapping run id is a valid source."}, pagedHandler(store.TypeAnswers)) - addTool(s, &mcp.Tool{Name: "get_history", Description: "Download a run's interactive state history into a dataset."}, + addTool(s, &mcp.Tool{Name: "get_history", Description: "Download a run's interactive state history into a dataset. A Student ID Mapping run id is a valid source."}, pagedHandler(store.TypeHistory)) - addTool(s, &mcp.Tool{Name: "get_attachments", Description: "Download a run's file attachments into a dataset. Fetch that run's answers or history first: attachments are reached through those records. " + noArgsMsg}, + addTool(s, &mcp.Tool{Name: "get_attachments", Description: "Download a run's file attachments into a dataset. Fetch that run's answers or history first: attachments are reached through those records. A Student ID Mapping run id is a valid source. " + noArgsMsg}, func(ctx context.Context, req *mcp.CallToolRequest, in getAttachmentsIn) (*mcp.CallToolResult, mapOut, error) { d, client, err := openForFetch(in.Dataset) if err != nil { From 298ea36c82ff96661ac2474ab0774c382939d57f Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 09:02:39 -0400 Subject: [PATCH 06/12] docs: reconcile the guide and the logs entry with the two families [REPORT-95] The expected outcome here was a small diff or none, and it was small: REPORT-94 already gave the guide a full Portal treatment, so it already named --refresh and both mapping slugs. Two things were left. The guide's run-creation section offered only an Athena example, so a researcher reading it would not learn that a whole cohort can be pulled without authoring an Athena report at all. It now points at the mapping path the guide already describes further down. The same section said a Portal report is re-read with get report, without the flag that does it, which the section two hundred lines later does name. The logs entry names three slugs inline, and now that the core defines two report families it has to sit visibly under one. It says they are Athena. --- docs/researcher-guide.md | 8 ++++++-- internal/guidance/src/core.md | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/researcher-guide.md b/docs/researcher-guide.md index efd2ec1..ef91e05 100644 --- a/docs/researcher-guide.md +++ b/docs/researcher-guide.md @@ -243,8 +243,12 @@ cc-data reports create --report-slug student-answers --report-filter '{"cohort": Use `--report-filter-file ` when the filter is too long to quote. To take a fresh snapshot of a run you already have, `cc-data reports duplicate `. -Portal reports are computed live, so re-reading one with `get report` gives you -current data and duplicating one needs `--force`. +Portal reports are computed live, so re-reading one with `get report --refresh` +gives you current data and duplicating one needs `--force`. + +To pull a whole cohort's work without authoring an Athena report at all, create a +`student-id-mapping` run and fetch answers, history and attachments by its run +id; the two Portal reports below describe that path. ### A complete session diff --git a/internal/guidance/src/core.md b/internal/guidance/src/core.md index a8ce9c3..1e89bde 100644 --- a/internal/guidance/src/core.md +++ b/internal/guidance/src/core.md @@ -84,8 +84,8 @@ like `wildfire_2026.answers`): (`student_name::VARCHAR`) when comparing or grouping it across runs. - `report_prompts` — the prompt and correct-answer text keyed by the `res___*` columns. -- `logs`: the log-type report CSVs (`student-actions`, - `student-actions-with-metadata`, `teacher-actions`) unioned with `run_id`, plus +- `logs`: the log-type report CSVs, all Athena (`student-actions`, + `student-actions-with-metadata`, `teacher-actions`), unioned with `run_id`, plus four parsed columns. The original `parameters`, `extras`, `time` and `timestamp` columns are retained unchanged alongside them. `parameters_json` and `extras_json` are the payload and the UI-state snapshot as From d4c7f9138705befb91cda5663532a8d7f3e95599 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 09:05:16 -0400 Subject: [PATCH 07/12] refactor: name the Athena slug accessor for what it returns [REPORT-95] ReportSlugs returns slugToType's keys, which is the Athena half and not every slug a report can be created from; the call site unions it with the dimension half. The name promised the whole set. The plan now also records two things the implementation settled: the download vocabulary's guard ships with the sentence it checks rather than with its sibling, so no commit lands red, and the accessors are exported for the same reason StaticViewNames and IdentityColumnNames already are. --- internal/dataset/reporttype.go | 5 +++-- internal/guidance/guard_test.go | 2 +- .../implementation.md | 15 ++++++++++++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/internal/dataset/reporttype.go b/internal/dataset/reporttype.go index 58d560c..52d1bb4 100644 --- a/internal/dataset/reporttype.go +++ b/internal/dataset/reporttype.go @@ -73,8 +73,9 @@ func IsAllowedReportType(t string) bool { return false } -// ReportSlugs returns the Athena slugs cc-data knows. -func ReportSlugs() []string { +// AthenaReportSlugs returns the Athena slugs cc-data knows. It is not every slug a +// report can be created from: the dimension views carry the two Portal ones. +func AthenaReportSlugs() []string { var out []string for slug := range slugToType { out = append(out, slug) diff --git a/internal/guidance/guard_test.go b/internal/guidance/guard_test.go index 732f6dc..0d9ea78 100644 --- a/internal/guidance/guard_test.go +++ b/internal/guidance/guard_test.go @@ -207,7 +207,7 @@ func TestGuidanceDocumentsOnlyRealSlugs(t *testing.T) { if err != nil { t.Fatal(err) } - inCode := append(dataset.ReportSlugs(), duck.DimensionSlugs()...) + inCode := append(dataset.AthenaReportSlugs(), duck.DimensionSlugs()...) if m := guidance.Missing(documented, inCode); len(m) > 0 { t.Fatalf("guidance names slugs the code does not know: %v", m) } diff --git a/specs/REPORT-95-portal-reports-guidance/implementation.md b/specs/REPORT-95-portal-reports-guidance/implementation.md index 1362bba..65f4636 100644 --- a/specs/REPORT-95-portal-reports-guidance/implementation.md +++ b/specs/REPORT-95-portal-reports-guidance/implementation.md @@ -87,6 +87,13 @@ exemption in the test. Both exemption sets ship empty, since this story states b full. They exist so the next value added has to make a decision rather than be forgotten, and so the decision is recorded beside the guard rather than in a commit message. +Only the run guard lands in this step. The download guard was written here and confirmed to fail +for the right reason, that the core states no such sentence yet, and then moved to the step that +adds the sentence: writing a guard before its prose is the point, but a commit that lands red is +not. Both accessors the guards read are exported for the same reason `StaticViewNames` and +`IdentityColumnNames` already are, since the guards live in a test package that cannot reach +package internals. + --- ## Teach the core the Portal report family, the slugs and the recipe @@ -96,9 +103,11 @@ the decision is recorded beside the guard rather than in a commit message. **Files affected**: - `internal/guidance/src/core.md` - `internal/guidance/guard_test.go` — the slug guard -- `internal/duck/views.go` and `internal/dataset/reporttype.go` — accessors for the two slug - inventories, both of which are unexported today (`dimensionViews[].slug` and `slugToType`'s keys), - so the guard can read either +- `internal/duck/views.go` and `internal/dataset/reporttype.go` — `DimensionSlugs` and + `AthenaReportSlugs`, accessors for the two slug inventories, both of which are unexported today + (`dimensionViews[].slug` and `slugToType`'s keys), so the guard can read either. The second is + named for what it returns rather than `ReportSlugs`, which would overpromise: it is the Athena + half, and the call site unions it with the dimension half. **Estimated diff size**: ~180 lines From 0c2a49effdf7dc8410c1f42af878c485eb4cf488 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 09:48:16 -0400 Subject: [PATCH 08/12] feat: let the skill surface act on the slugs it now lists [REPORT-95] skill_header.md named no cc-data reports subcommand at all, so a CLI-driving model would read the new slug catalog, read the recipe's opening step of creating a student-id-mapping run and assembling its filter, and have no command for either. Steps 1 and 4 of the six-step recipe were unexecutable on that surface, which is this story's own defect one level up: knowing the string and not the verb is as unexecutable as not knowing the string. The MCP surface needed nothing, since tools.md already names both tools. The header gains reports list, filter-options, create and duplicate, every spelling checked against its own --help. That caught one of mine: reports duplicate requires --portal and the first draft omitted it, which is exactly the failure the slug guard exists to prevent, one layer out. A test holds the skill surface to the two verbs the recipe needs, and asserts nothing about the MCP side, because the shipped tool guard already fails if either tool leaves the catalog, checked by removing one. Three fixes from the review pass. The recipe's materialize step used a section title as a bare noun phrase, so "when Materializing a dataset says it is worth it" had no coherent subject; it now names the section, as the step below it already did for the view entry. The researcher guide names the Report types section rather than pointing "below", which survives reordering. And the slugToType comment drops its closing ticket reference: the constraint it states is complete without one, and no other comment in the tree cites a ticket. The spec records the gap, the decision behind the ticket-free comment, and the finished surface sizes, which had been pinned to a commit two merges behind. --- docs/researcher-guide.md | 2 +- internal/dataset/reporttype.go | 3 +-- internal/guidance/guard_test.go | 14 ++++++++++++++ internal/guidance/src/core.md | 4 ++-- internal/guidance/src/skill_header.md | 12 ++++++++++++ .../implementation.md | 9 +++++++-- .../requirements.md | 6 ++++++ 7 files changed, 43 insertions(+), 7 deletions(-) diff --git a/docs/researcher-guide.md b/docs/researcher-guide.md index ef91e05..cdb588f 100644 --- a/docs/researcher-guide.md +++ b/docs/researcher-guide.md @@ -248,7 +248,7 @@ gives you current data and duplicating one needs `--force`. To pull a whole cohort's work without authoring an Athena report at all, create a `student-id-mapping` run and fetch answers, history and attachments by its run -id; the two Portal reports below describe that path. +id; the Report types section describes that path. ### A complete session diff --git a/internal/dataset/reporttype.go b/internal/dataset/reporttype.go index 52d1bb4..211ae6b 100644 --- a/internal/dataset/reporttype.go +++ b/internal/dataset/reporttype.go @@ -18,8 +18,7 @@ const ( // slugToType is cc-data's own copy of the Athena slugs, not a roster of what the server // offers. Nothing reconciles it: an unrecognized slug degrades with "unknown to this // cc-data version" rather than failing, and the guidance guard can only prove the guidance -// matches this map, never that this map matches the server. REPORT-130 replaces the copy -// with a catalog read at runtime. +// matches this map, never that this map matches the server. var slugToType = map[string]string{ "student-answers": ReportTypeAnswers, "student-assignment-usage": ReportTypeUsage, diff --git a/internal/guidance/guard_test.go b/internal/guidance/guard_test.go index 0d9ea78..7c013dc 100644 --- a/internal/guidance/guard_test.go +++ b/internal/guidance/guard_test.go @@ -218,6 +218,20 @@ func TestGuidanceDocumentsOnlyRealSlugs(t *testing.T) { // inventory comparison can notice one of them losing it. Only the per-surface halves are // asserted: the rule itself is in the core, which both render by construction, so // checking it here would be true by construction and would test nothing. +// The core lists the slugs and the recipe opens by creating a run from one, so the +// skill has to carry the verb that acts on a slug or the recipe's first step is +// unexecutable there. The core cannot carry it, the no-commands guard forbids it, and +// nothing else checks this file's command spellings. The MCP side needs no assertion +// here: TestGuidanceDocumentsEveryTool already fails if either tool leaves tools.md. +func TestSkillSurfaceCanActOnASlug(t *testing.T) { + if !strings.Contains(guidance.Skill(), "cc-data reports create --report-slug") { + t.Error("skill: no command creates a run from a slug") + } + if !strings.Contains(guidance.Skill(), "cc-data reports filter-options") { + t.Error("skill: no command assembles the filter the recipe's first step needs") + } +} + func TestBothSurfacesCarryTheRePullMechanism(t *testing.T) { if !strings.Contains(guidance.Skill(), "--refresh") { t.Error("skill: the --refresh spelling is missing") diff --git a/internal/guidance/src/core.md b/internal/guidance/src/core.md index 1e89bde..52a0fb7 100644 --- a/internal/guidance/src/core.md +++ b/internal/guidance/src/core.md @@ -205,8 +205,8 @@ their slugs come from an existing run or from the researcher guide. 3. Fetch the run's own report CSV, which becomes `student_id_mapping`. 4. Create and fetch a `student-metadata` run over the same learners, which becomes `student_metadata`. -5. Materialize the dataset before querying it when Materializing a dataset says - it is worth it. +5. Materialize the dataset before querying it, when the Materializing a dataset + section says it is worth doing. 6. Query: `answers` joins `student_id_mapping` on `run_remote_endpoint = remote_endpoint`, and `student_id_mapping` joins `student_metadata` on `learner_id`. See the `student_id_mapping` entry for what a NULL join key diff --git a/internal/guidance/src/skill_header.md b/internal/guidance/src/skill_header.md index b808689..69cf3a4 100644 --- a/internal/guidance/src/skill_header.md +++ b/internal/guidance/src/skill_header.md @@ -38,6 +38,18 @@ guessing flags. - The environment names also work wherever a `portal` is passed: the `--portal` flag on `logout` and on every `reports` subcommand. +## Making a run + +- `cc-data reports list --portal ` — the runs you already have. +- `cc-data reports filter-options --dimension --portal ` — the + values a filter dimension offers, narrowed by `--report-filter` as selections + are made, and by `--report-slug` to a report that offers the dimension. +- `cc-data reports create --report-slug --report-filter '' --portal + ` — a run from a slug and a filter. Use `--report-filter-file` when + the filter is too long to quote. +- `cc-data reports duplicate --portal ` — a fresh snapshot of + an Athena run. + ## Fetching data - `cc-data get report --dataset ` — the report CSV. A Portal report diff --git a/specs/REPORT-95-portal-reports-guidance/implementation.md b/specs/REPORT-95-portal-reports-guidance/implementation.md index 65f4636..2bb9973 100644 --- a/specs/REPORT-95-portal-reports-guidance/implementation.md +++ b/specs/REPORT-95-portal-reports-guidance/implementation.md @@ -151,7 +151,8 @@ from the researcher guide. 3. Fetch the run's own report CSV, which becomes `student_id_mapping`. 4. Create and fetch a `student-metadata` run over the same learners, which becomes `student_metadata`. -5. Materialize the dataset before querying it when Materializing a dataset says it is worth it. +5. Materialize the dataset before querying it, when the Materializing a dataset + section says it is worth doing. 6. Query: `answers` joins `student_id_mapping` on `run_remote_endpoint = remote_endpoint`, and `student_id_mapping` joins `student_metadata` on `learner_id`. See the `student_id_mapping` entry for what a NULL join key means before filtering on it. @@ -187,9 +188,11 @@ carries a one-line pointer to it: // slugToType is cc-data's own copy of the Athena slugs, not a roster of what the server offers. // Nothing reconciles it: an unrecognized slug degrades with "unknown to this cc-data version" // rather than failing, and the guidance guard can only prove the guidance matches this map, never -// that this map matches the server. REPORT-130 replaces the copy with a catalog read at runtime. +// that this map matches the server. ``` +The comment names no ticket, deliberately. The constraint it states is complete without one, and REPORT-130's replacement of this map is planning state rather than something the code cannot express; no other comment in the tree cites a ticket. + Verified that the union covers the documented set exactly: seven slugs in code, seven documented, none documented that code does not know. So the guard passes on the prose this plan writes, rather than being written and then having the prose trimmed to satisfy it. --- @@ -209,6 +212,8 @@ Verified that the union covers the documented set exactly: seven slugs in code, `skill_header.md` gains `--refresh` on the `get report` line, since the core states the rule and cannot state the flag. +It also gains a `## Making a run` section, which the plan originally missed. The file had no `cc-data reports` subcommand at all, so the recipe's first step, creating a run from a slug and a filter, was unexecutable on the surface that drives the CLI: the model would know `student-id-mapping` and have no verb to use it with. The MCP surface needed nothing, since `tools.md` already names `reports_create` and `reports_filter_options`. A test holds the skill surface to it, in the same shape as the re-pull assertion and for the same reason: the core cannot carry a command spelling, so no comparison of the core can notice this file losing one. It asserts nothing about the MCP surface, because the shipped tool guard already fails if either tool leaves `tools.md`, which was checked by removing one. + `tools.md`'s `get_report` entry gains `refresh` too, and this is not redundant with the tool description. A tool's `Description` is registered with the MCP server and is **not** part of `guidance.Instructions()`, which renders `mcp_header.md` + `core.md` + `tools.md` only. Verified: none of three distinctive description strings appears in the rendered instructions. So the MCP surface's *guidance* learns about `refresh` only if `tools.md` says so, and a both-surfaces test that looked for it in `Instructions()` without this edit would fail on the day it was written. Six MCP descriptions gain one named fact each, rather than a general instruction to mention Portal runs: diff --git a/specs/REPORT-95-portal-reports-guidance/requirements.md b/specs/REPORT-95-portal-reports-guidance/requirements.md index 7b00ed4..1ef35b6 100644 --- a/specs/REPORT-95-portal-reports-guidance/requirements.md +++ b/specs/REPORT-95-portal-reports-guidance/requirements.md @@ -62,6 +62,7 @@ The MCP descriptions for `reports_list`, `get_report`, `get_answers`, `get_histo - The end-to-end recipe is documented: create a Student ID Mapping run, pull answers, history and attachments by its run id, download the Student ID Mapping and Student Metadata CSVs, materialize first if the pull is large, then query, joining answers and history on `remote_endpoint` and Student Metadata on `learner_id`. - The recipe's materialize step is a **conditional pointer**, not a restatement: REPORT-113 owns when materializing is worth it, and this step references that rule rather than repeating the condition. The step exists because REPORT-115 templates its CLUE workflow on this recipe and puts materialize in exactly this position ("materialize (REPORT-113) when the history store is large"), for a corpus where it is closest to mandatory. A recipe with no slot for it would force 115 to invent one and the two workflows to diverge structurally. - The re-pull mechanism is named on both surfaces: `--refresh` for the CLI, the `refresh` parameter for the MCP tool. +- Each surface carries the verb that acts on a slug, or the recipe's first step is unexecutable there. The MCP surface already had it, since `tools.md` names `reports_create` and `reports_filter_options`; the skill surface named no `cc-data reports` subcommand at all, so a CLI-driving model could read the slug catalog and the recipe and still have no way to create a run or assemble a filter. That is the same defect this story was filed for, one level up: knowing the string and not the command is as unexecutable as not knowing the string. The skill header gains `reports list`, `reports filter-options`, `reports create` and `reports duplicate`, every spelling checked against its own `--help`, and a test holds the skill surface to it. The MCP side needs no new assertion, since the shipped tool guard already fails if either tool leaves the catalog. - The `student_id_mapping` and `student_metadata` view entries gain exactly the two facts they lack and are otherwise left alone: the slug a run of each is created from, and that such a run's id is a valid source for fetching answers, history and attachments. They already carry the dedup rule, the withheld-join-key rule and the `hide_names` rule. - The MCP descriptions carry named facts rather than a general instruction to mention Portal runs, and a test asserts each one, since the drift guard checks tool names and never looks at description text: `reports_list` says a run's execution tells Athena from Portal; `reports_filter_options` says it is how a Student ID Mapping run's filter is assembled, since the recipe's first step sends the model straight to it; `get_report` says a Portal report is re-read with `refresh` rather than duplicated; and `get_answers`, `get_history` and `get_attachments` each say a Student ID Mapping run id is a valid source. - A guard holds that every report slug named in the guidance exists in the code's slug inventories, so a typo cannot ship a slug that fails at the server. @@ -128,6 +129,11 @@ nothing that parses today.** Measured over the rendered surfaces: Views 12 befor earlier check had only REPORT-112's branch; Tools is 21 because REPORT-113's `dataset_materialize` entry is now in the catalog, and it parses the same either way. +**Measured on the finished implementation (2026-09-11):** core 17,002 bytes, skill 20,319, MCP +instructions 20,503. This story added 2,668 to the core, 3,413 to the skill and 2,781 to the +instructions, against a baseline that had itself grown 39% since the spec was drafted. The skill +grows most because it alone carries the command spellings the core is forbidden to name. + **Moved: every byte measurement, by more than the recipe it was sizing.** Re-measured on this branch: core 14,334 bytes, skill 16,906, MCP instructions 17,722, against the 10,292 / 12,464 / 13,462 the spec records. The core grew 39%. The decision the numbers support is unchanged and From 8efb052686486f236817262dc3cd43948bb5712b Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 09:56:01 -0400 Subject: [PATCH 09/12] test: hold the guidance's command spellings to the command tree [REPORT-95] The guidance spells out cc-data commands because the core is forbidden to, and nothing compared those spellings against the commands that exist. A renamed or removed subcommand would leave the skill telling a researcher to run something that is gone. The guard walks every backticked invocation in both rendered surfaces and asserts it resolves. cobra.Find alone is not enough, and the first version of this guard passed on two of three deliberate breaks because of it: Find returns the deepest command it matched plus the args it could not consume, and errors only when the first word is unknown, so `reports clone` resolves to `reports` with a leftover. The leftovers are the signal. Names only, deliberately. Flags cannot be guarded the same way: --portal is not a cobra-required flag but a runtime fallback to default_portal, so there is nothing declarative to compare against. That same fact corrects the four lines this branch added. They carried --portal, on the mistaken belief that omitting it was an error; it is needed only when no default portal is configured, which the rest of the file already handles by showing what is unconditionally required and nothing more. The flag is named once as a condition instead of four times as a requirement. --- cmd/guidance_commands_test.go | 52 +++++++++++++++++++ internal/guidance/src/skill_header.md | 19 +++---- .../implementation.md | 4 +- .../requirements.md | 3 +- 4 files changed, 67 insertions(+), 11 deletions(-) create mode 100644 cmd/guidance_commands_test.go diff --git a/cmd/guidance_commands_test.go b/cmd/guidance_commands_test.go new file mode 100644 index 0000000..7bca0fd --- /dev/null +++ b/cmd/guidance_commands_test.go @@ -0,0 +1,52 @@ +package cmd + +import ( + "regexp" + "strings" + "testing" + + "github.com/concord-consortium/cc-data-cli/internal/guidance" +) + +// commandRefs matches a backticked cc-data invocation in the guidance, capturing +// the subcommand words that follow. It stops at the first token that is not a +// bare word, so flags, placeholders and quoted JSON are left out. +var commandRefs = regexp.MustCompile("`cc-data((?: [a-z][a-z-]*)+)") + +// The guidance spells commands out because the core is forbidden to, so nothing +// else compares those spellings against the commands that exist. A renamed or +// removed subcommand would otherwise leave the guidance telling a researcher to +// run something that is gone. Names only: flags are not checked, because a flag +// like --portal is required only when no default portal is configured, so there +// is nothing declarative to compare against. +func TestGuidanceNamesOnlyRealCommands(t *testing.T) { + root := newRootCmd() + seen := map[string]bool{} + // Both rendered surfaces: the skill spells out the CLI, and the MCP header + // relays the login remedy, which is the one command an agent passes on. + both := guidance.Skill() + "\n" + guidance.Instructions() + for _, m := range commandRefs.FindAllStringSubmatch(both, -1) { + words := strings.Fields(m[1]) + if seen[strings.Join(words, " ")] { + continue + } + seen[strings.Join(words, " ")] = true + // Find returns the deepest command it matched plus the args it could not + // consume, and errors only when the first word is unknown, so a wrong + // subcommand resolves to its parent with leftovers. The leftovers are the + // signal. + found, rest, err := root.Find(words) + if err != nil { + t.Errorf("the skill names `cc-data %s`, which is not a command: %v", strings.Join(words, " "), err) + continue + } + if len(rest) > 0 { + t.Errorf("the skill names `cc-data %s`, but %q is not a subcommand of %q", + strings.Join(words, " "), rest[0], found.CommandPath()) + } + } + if len(seen) == 0 { + t.Fatal("no cc-data invocations found in the guidance, so this checks nothing") + } + t.Logf("checked %d distinct invocations", len(seen)) +} diff --git a/internal/guidance/src/skill_header.md b/internal/guidance/src/skill_header.md index 69cf3a4..0f974d4 100644 --- a/internal/guidance/src/skill_header.md +++ b/internal/guidance/src/skill_header.md @@ -40,15 +40,16 @@ guessing flags. ## Making a run -- `cc-data reports list --portal ` — the runs you already have. -- `cc-data reports filter-options --dimension --portal ` — the - values a filter dimension offers, narrowed by `--report-filter` as selections - are made, and by `--report-slug` to a report that offers the dimension. -- `cc-data reports create --report-slug --report-filter '' --portal - ` — a run from a slug and a filter. Use `--report-filter-file` when - the filter is too long to quote. -- `cc-data reports duplicate --portal ` — a fresh snapshot of - an Athena run. +- `cc-data reports list` — the runs you already have. +- `cc-data reports filter-options --dimension ` — the values a filter + dimension offers, narrowed by `--report-filter` as selections are made, and by + `--report-slug` to a report that offers the dimension. +- `cc-data reports create --report-slug --report-filter ''` — a run + from a slug and a filter. Use `--report-filter-file` when the filter is too + long to quote. +- `cc-data reports duplicate ` — a fresh snapshot of an Athena run. + +These take `--portal` when no default portal is configured. ## Fetching data diff --git a/specs/REPORT-95-portal-reports-guidance/implementation.md b/specs/REPORT-95-portal-reports-guidance/implementation.md index 2bb9973..5da8366 100644 --- a/specs/REPORT-95-portal-reports-guidance/implementation.md +++ b/specs/REPORT-95-portal-reports-guidance/implementation.md @@ -212,7 +212,9 @@ Verified that the union covers the documented set exactly: seven slugs in code, `skill_header.md` gains `--refresh` on the `get report` line, since the core states the rule and cannot state the flag. -It also gains a `## Making a run` section, which the plan originally missed. The file had no `cc-data reports` subcommand at all, so the recipe's first step, creating a run from a slug and a filter, was unexecutable on the surface that drives the CLI: the model would know `student-id-mapping` and have no verb to use it with. The MCP surface needed nothing, since `tools.md` already names `reports_create` and `reports_filter_options`. A test holds the skill surface to it, in the same shape as the re-pull assertion and for the same reason: the core cannot carry a command spelling, so no comparison of the core can notice this file losing one. It asserts nothing about the MCP surface, because the shipped tool guard already fails if either tool leaves `tools.md`, which was checked by removing one. +It also gains a `## Making a run` section, which the plan originally missed. The file had no `cc-data reports` subcommand at all, so the recipe's first step, creating a run from a slug and a filter, was unexecutable on the surface that drives the CLI: the model would know `student-id-mapping` and have no verb to use it with. The MCP surface needed nothing, since `tools.md` already names `reports_create` and `reports_filter_options`. A second test, in `package cmd` because it needs the cobra tree, walks every backticked `cc-data ` in both rendered surfaces and asserts it resolves. `root.Find` is not enough on its own: it returns the deepest command it matched plus the args it could not consume, and errors only when the *first* word is unknown, so `reports clone` silently resolves to `reports` with a leftover. The leftovers are the signal, and without checking them the guard passes on a renamed subcommand. + +A test holds the skill surface to it, in the same shape as the re-pull assertion and for the same reason: the core cannot carry a command spelling, so no comparison of the core can notice this file losing one. It asserts nothing about the MCP surface, because the shipped tool guard already fails if either tool leaves `tools.md`, which was checked by removing one. `tools.md`'s `get_report` entry gains `refresh` too, and this is not redundant with the tool description. A tool's `Description` is registered with the MCP server and is **not** part of `guidance.Instructions()`, which renders `mcp_header.md` + `core.md` + `tools.md` only. Verified: none of three distinctive description strings appears in the rendered instructions. So the MCP surface's *guidance* learns about `refresh` only if `tools.md` says so, and a both-surfaces test that looked for it in `Instructions()` without this edit would fail on the day it was written. diff --git a/specs/REPORT-95-portal-reports-guidance/requirements.md b/specs/REPORT-95-portal-reports-guidance/requirements.md index 1ef35b6..edc8ba3 100644 --- a/specs/REPORT-95-portal-reports-guidance/requirements.md +++ b/specs/REPORT-95-portal-reports-guidance/requirements.md @@ -62,7 +62,8 @@ The MCP descriptions for `reports_list`, `get_report`, `get_answers`, `get_histo - The end-to-end recipe is documented: create a Student ID Mapping run, pull answers, history and attachments by its run id, download the Student ID Mapping and Student Metadata CSVs, materialize first if the pull is large, then query, joining answers and history on `remote_endpoint` and Student Metadata on `learner_id`. - The recipe's materialize step is a **conditional pointer**, not a restatement: REPORT-113 owns when materializing is worth it, and this step references that rule rather than repeating the condition. The step exists because REPORT-115 templates its CLUE workflow on this recipe and puts materialize in exactly this position ("materialize (REPORT-113) when the history store is large"), for a corpus where it is closest to mandatory. A recipe with no slot for it would force 115 to invent one and the two workflows to diverge structurally. - The re-pull mechanism is named on both surfaces: `--refresh` for the CLI, the `refresh` parameter for the MCP tool. -- Each surface carries the verb that acts on a slug, or the recipe's first step is unexecutable there. The MCP surface already had it, since `tools.md` names `reports_create` and `reports_filter_options`; the skill surface named no `cc-data reports` subcommand at all, so a CLI-driving model could read the slug catalog and the recipe and still have no way to create a run or assemble a filter. That is the same defect this story was filed for, one level up: knowing the string and not the command is as unexecutable as not knowing the string. The skill header gains `reports list`, `reports filter-options`, `reports create` and `reports duplicate`, every spelling checked against its own `--help`, and a test holds the skill surface to it. The MCP side needs no new assertion, since the shipped tool guard already fails if either tool leaves the catalog. +- Every `cc-data` command the guidance spells out resolves to a real command, checked across both rendered surfaces. The guidance carries the spellings precisely because the core is forbidden to, so nothing else compares them against the command tree, and a renamed subcommand would leave the skill telling a researcher to run something that is gone. Names only: flags are deliberately not checked, since a flag like `--portal` is required only when no default portal is configured and there is nothing declarative to compare against. +- Each surface carries the verb that acts on a slug, or the recipe's first step is unexecutable there. The MCP surface already had it, since `tools.md` names `reports_create` and `reports_filter_options`; the skill surface named no `cc-data reports` subcommand at all, so a CLI-driving model could read the slug catalog and the recipe and still have no way to create a run or assemble a filter. That is the same defect this story was filed for, one level up: knowing the string and not the command is as unexecutable as not knowing the string. The skill header gains `reports list`, `reports filter-options`, `reports create` and `reports duplicate`, and a test holds the skill surface to it. The lines show only what is unconditionally needed, matching the rest of the file: `--portal` is not a cobra-required flag but a runtime fallback to `default_portal` (`cmd/reports.go:35-43`), so it is named once as a condition rather than repeated on four lines a researcher with a default portal never needs it on. The MCP side needs no new assertion, since the shipped tool guard already fails if either tool leaves the catalog. - The `student_id_mapping` and `student_metadata` view entries gain exactly the two facts they lack and are otherwise left alone: the slug a run of each is created from, and that such a run's id is a valid source for fetching answers, history and attachments. They already carry the dedup rule, the withheld-join-key rule and the `hide_names` rule. - The MCP descriptions carry named facts rather than a general instruction to mention Portal runs, and a test asserts each one, since the drift guard checks tool names and never looks at description text: `reports_list` says a run's execution tells Athena from Portal; `reports_filter_options` says it is how a Student ID Mapping run's filter is assembled, since the recipe's first step sends the model straight to it; `get_report` says a Portal report is re-read with `refresh` rather than duplicated; and `get_answers`, `get_history` and `get_attachments` each say a Student ID Mapping run id is a valid source. - A guard holds that every report slug named in the guidance exists in the code's slug inventories, so a typo cannot ship a slug that fails at the server. From 5a7eaed6ddcdff7fd5afa44f98dd90d3081ba883 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 10:07:42 -0400 Subject: [PATCH 10/12] spec: close the Portal-reports guidance spec [REPORT-95] Replace the requirements and implementation files with a single closed spec holding the overview, the verified gap table, the 22 requirements, the technical notes and the decisions that shaped the work. The decisions worth carrying forward: the slug guard runs one direction only, because a slug the server offers and the code lacks is not a defect the guidance should block on; the core states the run vocabulary and the downloads entry states the download one, since the two differ and conflating them was the original requirement's error; and the guidance names the seven slugs it can prove while saying the list is not exhaustive, with the server-side catalog that removes the drift class filed as REPORT-130. --- specs/REPORT-95-portal-reports-guidance.md | 167 ++++++++++ .../implementation.md | 285 ------------------ .../requirements.md | 256 ---------------- 3 files changed, 167 insertions(+), 541 deletions(-) create mode 100644 specs/REPORT-95-portal-reports-guidance.md delete mode 100644 specs/REPORT-95-portal-reports-guidance/implementation.md delete mode 100644 specs/REPORT-95-portal-reports-guidance/requirements.md diff --git a/specs/REPORT-95-portal-reports-guidance.md b/specs/REPORT-95-portal-reports-guidance.md new file mode 100644 index 0000000..cff7111 --- /dev/null +++ b/specs/REPORT-95-portal-reports-guidance.md @@ -0,0 +1,167 @@ +# Integrate Portal reports into the Claude skill and MCP guidance + +**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-95 + +**Status**: **Closed** + +## Overview + +Teach Claude the Portal-report workflow in the shared guidance both surfaces render, so that a researcher's data question reaches for the right report kind and the whole pull can be driven end to end. The guidance documented the two Portal-fed views in detail but never named the report family, never gave the slugs needed to create a run, and never mentioned the flag that re-reads a live report. + +cc-data could already create Portal report runs, download them and join them to student answers. Claude could not reliably drive any of it, because the guidance it reads was written for the Athena-only world and had only been patched where individual stories touched it. A researcher asking Claude for class-level results got steered down the Athena path, which is slower, needs a report authored first, and returns a frozen snapshot. + +## Verified gaps + +Measured by rendering each surface and probing it, rather than by reading the sources: + +| Fact Claude needs | Skill | MCP instructions | +| --- | --- | --- | +| The phrase "Portal report" | **absent** | present | +| `student-id-mapping` slug | **absent** | **absent** | +| `student-metadata` slug | **absent** | **absent** | +| `--refresh` | **absent** | **absent** | +| `student_id_mapping` / `student_metadata` views | present | present | +| Re-pull rather than duplicate | present | present | + +**The ticket's background was out of date, and the spec was written against the code instead.** The ticket said the researcher guide "asserts the opposite in two places" and that both must be reversed. REPORT-94 had already removed both strings in `564da81`, replacing them with a full Portal treatment, and had landed the two dimension views the ticket calls "minimal stubs", which are in fact the longest entries in the views catalog. That relocated the work rather than removing it: the guide was in good shape and the *guidance* was where the holes were. + +## Requirements + +- The shared core names the Portal report family and the Athena/Portal distinction, so both surfaces carry it rather than the MCP surface alone. +- The run sentence at `core.md:24` keeps its list, which is correct for runs, and gains the fact it lacks: a Portal run has no `report_type`, and `execution` is what identifies it. +- The core carries the live-versus-snapshot model as a decision rule, not a fact: a Portal report is computed per request and is refreshed by re-pulling the same run; an Athena report is a frozen artifact and is refreshed by duplicating it into a new run. +- The guidance names every report slug the code knows: `student-id-mapping`, `student-metadata` and the five Athena slugs, so a run can be created from the guidance alone. +- The catalog says it is not exhaustive, in one sentence, because the Portal offers five aggregate metrics reports whose slugs are in no Go inventory and which no endpoint enumerates. +- What the slug guard proves, and what it cannot, is stated where each half can be acted on: the one-direction rule beside the guard, and the warning that cc-data's own inventory can be stale beside `slugToType`. +- The recipe names the joins but points at the rules that govern them rather than restating them. +- The end-to-end recipe is documented: create a Student ID Mapping run, pull answers, history and attachments by its run id, download both Portal CSVs, materialize if it is worth it, then query. +- The recipe's materialize step is a conditional pointer, not a restatement. *(REPORT-113 owns the rule; REPORT-115 templates its CLUE workflow on this recipe and needs the slot.)* +- The re-pull mechanism is named on both surfaces: `--refresh` for the CLI, the `refresh` parameter for the MCP tool. +- Every `cc-data` command the guidance spells out resolves to a real command, checked across both rendered surfaces. Names only: flags are deliberately not checked, since a flag like `--portal` is required only when no default portal is configured and there is nothing declarative to compare against. +- Each surface carries the verb that acts on a slug, or the recipe's first step is unexecutable there. +- The `student_id_mapping` and `student_metadata` view entries gain exactly the two facts they lack, the slug and that such a run's id drives the `get_*` verbs, and are otherwise left alone. +- The MCP descriptions carry named facts rather than a general instruction to mention Portal runs, and a test asserts each one. +- A guard holds that every report slug named in the guidance exists in the code's slug inventories. +- Each `report_type` vocabulary the guidance states is guarded against the code list that defines it, in both directions. There are two, and conflating them is what made the original requirement wrong. +- The `downloads` view entry states the download vocabulary its `report_type` column carries, which is where `portal` and `recovered` belong. +- Each vocabulary gets one readable definition in code, the way the view list already has one. +- `ParseCatalog`'s identifier pattern is widened to accept hyphens, without which a slug catalog parses as empty and its guard silently checks nothing. +- The re-pull mechanism is asserted on both surfaces by a test, following the auth remedy. Content placed in `core.md` needs no such test: both surfaces render the core by construction. +- REPORT-112's `logs` view entry is reconciled with the two-families framing this story introduces. +- The researcher guide is checked against the finished guidance and updated only where it is now wrong or silent. The acceptance criterion is that the three documents agree, not that the guide changed. + +## Technical Notes + +- Command spellings cannot go in the core: `TestCoreNamesNoCommand` rejects any "`cc-data `" string there. So the *rule* goes in the core and the spelling goes in `skill_header.md`, with the MCP surface carrying the parameter on the tool description. This is the same split the auth remedy already uses. +- Two code-side slug inventories exist and do not overlap: `dataset.slugToType` holds the five Athena slugs, and `duck.dimensionViews[].slug` holds the two Portal ones. The guard unions them. +- `ParseCatalog` reads a named section and pulls the backticked identifiers that open each bullet or table row, which is the shape any new guarded section has to take. It closes a section at the next heading of any level, so the recipe's `###` subheading ends the `## Report slugs` section; harmless as written, because the slug bullets precede it, and the ordering is now a stated constraint rather than an accident. +- The `report_type` guards cannot use `ParseCatalog`, because the sentences carrying the vocabularies open with prose rather than a backticked identifier. Matching `` `report_type` (...) `` against the rendered core extracts each list from its sentence as written, so the guards cost no restructuring and no bytes. The core is hard-wrapped, so the match has to tolerate a list spanning lines. +- A tool's `Description` is registered with the MCP server and is **not** part of `guidance.Instructions()`, which renders `mcp_header.md` + `core.md` + `tools.md` only. So the MCP surface's *guidance* learns about `refresh` only if `tools.md` says so. +- `cobra.Find` returns the deepest command it matched plus the args it could not consume, and errors only when the first word is unknown, so a wrong subcommand resolves to its parent with leftovers. The leftovers are the signal the command guard keys on. +- Measured on the finished implementation: core 17,018 bytes, skill 20,306, MCP instructions 20,519. This story added 2,684 / 3,400 / 2,797 against a baseline that had itself grown 39% since the spec was drafted. The skill grows most because it alone carries the command spellings the core is forbidden to name. + +## Out of Scope + +- **Writing the "when to materialize" rule.** REPORT-113 owns that prose. This story references the rule from the recipe and must not restate the condition. +- **CLUE document guidance.** REPORT-115 owns that, and its description names this story's prose as its template. +- Rewriting the researcher guide's Portal-reports treatment, which REPORT-94 landed. +- Any change to the views themselves, or to what `reports_create` accepts. +- Naming the five aggregate Portal metrics reports. Their slugs are in no Go inventory and no endpoint enumerates them; the catalog says so in one sentence instead. *(Filed as REPORT-130.)* + +## Decisions + +### Should the slug guard be bidirectional, and what happens to the aggregate reports? + +**Context**: A guard that every guidance-named slug exists in code would have caught the fact that no slug was documented at all. But the two code inventories cover only seven slugs, and the Portal aggregate reports have slugs in neither. + +**Options considered**: +- A) One direction only: every slug the guidance names must exist in a code inventory. +- B) Bidirectional, with the aggregate slugs added to a Go inventory first. +- C) No guard; the slugs are few and the researcher guide already lists them. + +**Decision**: A. A wrong slug fails at the server with an error that says nothing about spelling, and that is the failure worth catching; the code having no constant for a report the server offers is not a defect the guidance should be blocked on. The discovery that made this actionable: `ParseCatalog`'s `leadingNames` regex excluded hyphens, so a slug catalog parsed as empty and the guard would have reported success while checking nothing. Widening it is a prerequisite, and the four shipped catalogs parse identically before and after. + +--- + +### How much of the recipe belongs in the core, given the context budget? + +**Context**: The core is rendered into every session on both surfaces, and REPORT-89 is separately trying to protect the model's context budget. + +**Options considered**: +- A) Full recipe in the core. +- B) Decision rule in the core, worked example in the researcher guide only. +- C) Full recipe now, and let REPORT-89's cold walk-through decide whether it earns its bytes. + +**Decision**: A, and the measurement decides it. Both versions were drafted and measured against the real file: the minimal decision rule is +765 bytes, the full recipe +1,532. That is not where a context budget is won or lost, and the story exists precisely so the model can execute the workflow rather than narrate it. A and C are compatible rather than alternatives. + +--- + +### Does this story still own researcher-guide work? + +**Context**: The ticket's guide requirement was to reverse two assertions, which REPORT-94 already did. + +**Options considered**: +- A) Yes, narrowed to a consistency pass. +- B) No; drop the guide and note that REPORT-94 discharged it. + +**Decision**: A. The guide is the only one of the three documents a human reads end to end, and this story adds slugs and a workflow to the other two. The acceptance criterion is that the three documents agree, not that the guide was edited; a pass finding nothing is a passing outcome. + +--- + +### Which `report_type` vocabulary does the core's run sentence state? + +**Context**: The original requirement said the sentence "gains `portal`, so the vocabulary it states is the vocabulary the code accepts". A later review found that conflates two different vocabularies. + +**Options considered**: +- A) Add `portal` (and `recovered`) to the run sentence, guarded against `IsAllowedReportType`. +- B) Keep the run sentence's list, add the missing discriminator, and state the download vocabulary where downloads are described, each guarded against the list that defines it. + +**Decision**: B. Measured, the two sets differ: a run carries `[answers log usage]` and nothing else, because a Portal run's `report_type` is null on the wire, pinned by `portalRunWire` and stated as design in the API type; the reports union additionally admits `portal` and `recovered`, both assigned locally. Adding `portal` to a sentence about runs would have told the model a Portal run carries `report_type: portal` while this story's own `reports_list` description teaches that `execution` is the discriminator. The gap was never a missing enum value, it was a missing discriminator. + +--- + +### Hand-fix the drifted vocabulary, or guard it? + +**Context**: This story exists because `core.md`'s report-type list drifted from the code. + +**Options considered**: +- A) Add the missing values by hand. +- B) Guard each vocabulary against the code list that defines it, in both directions, with an explicit exemption list. + +**Decision**: B. Correcting a list by hand answers today's question and leaves the next value free to drift, in the one story whose subject is guidance that drifted, and next to five guards that already do this for views, tools, identity columns, the auth remedy and command spellings. `IsAllowedReportType` was a `switch`, which offers a guard nothing to read, so the allowlist became a slice the predicate consults. The exemption sets ship empty and exist so a value added later has to make a decision rather than be forgotten. + +--- + +### Should the guidance hardcode a slug list at all? + +**Context**: The server owns which reports exist, and every local copy is a cache with no invalidation. `slugToType` is a static map nothing reconciles, and cc-data already warns "unknown to this cc-data version" on a slug it lacks. + +**Options considered**: +- A) List the aggregate slugs too, widening or exempting the guard. +- B) State the boundary in one sentence without the slugs. +- C) Out of scope, filed. + +**Decision**: B now, with the end state filed as REPORT-130. The repo already refused a slug map once for this reason: deriving a report's type from `execution` rather than a slug lookup is what lets "a Portal report added to the server later be recognized without a cc-data release". A was not available regardless, since three of the five slugs are not in this repo to copy. A server-side catalog the guidance points at, instead of enumerating, is the answer that removes the drift class entirely. + +--- + +### Should the guidance's command spellings be guarded, and how far? + +**Context**: The guidance spells out commands because the core is forbidden to, and nothing compared those spellings against the commands that exist. + +**Options considered**: +- A) No guard. +- B) Guard command names only. +- C) Guard names and flags. + +**Decision**: B. C is not available: `--portal` is not a cobra-required flag but a runtime fallback to `default_portal`, so there is nothing declarative to compare against, and the repo marks no flag required anywhere. That same fact settled how the commands are written: the lines show only what is unconditionally needed, matching the rest of the file, with `--portal` named once as a condition. The guard must check `Find`'s leftover args, not just its error, or a renamed subcommand resolves to its parent and the guard passes. + +--- + +### Resolved during review, and worth keeping + +- **"Enrich the stubs" named no actual gap.** The ticket's framing was that the two dimension-view entries are minimal stubs; they are the longest entries in the catalog. Fixed by naming the two facts they actually lack. An instruction to "enrich" would have invited a rewrite of correct prose. +- **"Mention Portal runs where relevant" was a requirement that could not fail.** The drift guard compares tool *names* and never reads a description. Fixed by naming the specific fact each description must carry and asserting each, which is a test with a mutation to catch. +- **A both-surfaces test on core content is decorative.** The core is rendered by both surfaces by construction, so asserting it on each is true no matter what. Only the per-surface halves, the `--refresh` spelling and the `refresh` parameter, need holding together. +- **The both-surfaces test would have failed on the day it was written.** `refresh` was planned for `skill_header.md` and the tool description only, and a tool's description is not part of the rendered instructions. The tempting fix, reading the registered tools instead, would have made it pass while no longer holding the thing it exists for. `tools.md` gains the parameter instead, which is a precondition rather than a nicety. +- **The four shipped catalogs need no new test for the regex widening.** The view, tool, identity-column and researcher-guide guards already compare each catalog against its code-derived set in both directions, so a name the widened pattern gained or lost fails one of them. A new test asserting the same comparison could not fail unless one of those four also failed. diff --git a/specs/REPORT-95-portal-reports-guidance/implementation.md b/specs/REPORT-95-portal-reports-guidance/implementation.md deleted file mode 100644 index 5da8366..0000000 --- a/specs/REPORT-95-portal-reports-guidance/implementation.md +++ /dev/null @@ -1,285 +0,0 @@ -# Implementation Plan: Portal reports in the Claude skill and MCP guidance - -**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-95 -**Requirements Spec**: [requirements.md](requirements.md) -**Status**: **In Development** - -## Shape of the change - -This is a documentation story with three code-shaped constraints, all verified: - -- **The core cannot name a command.** `TestCoreNamesNoCommand` rejects any "`cc-data `" string in `core.md`, so the recipe names operations ("create a `student-id-mapping` run") and the command spellings live in `skill_header.md`. The draft was run against the real guard and passes. -- **The slug guard needs a one-character regex change first.** `leadingNames` excludes hyphens, so a slug catalog parses as empty today and `ParseCatalog` reports success anyway. Verified that widening it leaves all four existing catalogs parsing to identical names. -- **Anything in `core.md` reaches both surfaces by construction; anything in `skill_header.md` or a tool description reaches one.** That decides where each fact goes and which facts need a both-surfaces test. - -Both inputs have landed in this branch's base as of 2026-09-11: it is stacked on `REPORT-113-dataset-materialization`, which carries REPORT-112's merge, so the `core.md` conflict the Precondition anticipated is already resolved and stage 4 has been re-run against the result. When REPORT-113 merges, rebase onto `main` before retargeting this PR, so its diff is this story's work alone; GitHub does not retarget a stacked child on its own, and this repo does not delete branches on merge, so the child is not at risk of being closed. - ---- - -## Widen the catalog identifier pattern - -**Summary**: Makes a hyphenated catalog possible. Independent of everything else and worth landing alone, because it changes a regex three shipped guards depend on. - -**Files affected**: -- `internal/guidance/catalog.go` — `leadingNames` -- `internal/guidance/catalog_test.go` — the identical-parse evidence - -**Estimated diff size**: ~60 lines - -```go -// leadingNames matches the backticked identifiers that open a catalog entry, in either -// markup the guarded files use: a bulleted list item ("- `x`, `y` — ...") or a markdown -// table row ("| `x`, `y` | ..."). Hyphens are allowed because report slugs carry them; -// without that a slug catalog parses as empty and its guard passes while checking nothing. -var leadingNames = regexp.MustCompile("^(?:- |\\| )((?:`[a-z0-9_-]+`(?:, )?)+)") -``` - -The property that matters is not that the new pattern accepts a hyphen, which is true by inspection. It is that the four shipped catalogs parse to the *same* names as before, so this cannot quietly change what the existing guards check. That property needs no new test: `TestGuidanceDocumentsEveryStaticView`, `TestGuidanceDocumentsEveryTool`, `TestGuidanceDocumentsEveryIdentityColumn` and `TestResearcherGuideDocumentsEveryStaticView` already compare each catalog against its code-derived set in both directions, so any name the widened pattern gained or lost fails one of them. A new test asserting the same comparison could not fail unless one of those four also failed. Measured before and after regardless: core Views 12 names, core Identity columns 4, tools Tools 21, researcher-guide table 12, all identical. - -A second test covers the failure this exists to prevent: a section of hyphenated names parses to all of them, where the old pattern yielded none and returned no error. - ---- - -## Guard the report-type vocabularies - -**Summary**: Gives each vocabulary one readable definition and holds the sentence that states it -against the right one. Lands before the core edit, so the core edit is what turns the guards green -rather than the guards being written to fit whatever the core happens to say. - -**Files affected**: -- `internal/dataset/reporttype.go`: the union vocabulary as a slice with `IsAllowedReportType` - reading it, and an accessor for the run vocabulary -- `internal/guidance/guard_test.go`: the two guards and their exemption lists - -**Estimated diff size**: ~120 lines - -There are two vocabularies and they are not the same set, which is the trap this step exists to -close. Measured: - -``` -run report_type (what the server sends on a run): [answers log usage] + null for Portal -union report_type (IsAllowedReportType, downloads): [answers usage log portal recovered] -``` - -The run set is `slugToType`'s values; `portal` and `recovered` are never on a run, they are -assigned locally to a download by `internal/fetch/report.go:162` and by reindex. A single guard -over `AllowedReportTypes()` would therefore force `portal` into a sentence about runs, which the -wire contradicts. - -```go -// AllowedReportTypes is the reports-union allowlist, in one place so the guidance -// guard and the predicate cannot disagree about what the vocabulary is. -func AllowedReportTypes() []string - -// RunReportTypes is what the server sends as a run's report_type. A Portal run -// sends none, which is why execution and not this list identifies one. -func RunReportTypes() []string -``` - -Neither guard can use `ParseCatalog`, since both sentences open with prose rather than a backticked -identifier. Neither needs to: matching `` `report_type` (...) `` against the rendered core returns -the list from the sentence as written. Verified against the current file, which yields `answers`, -`log`, `usage`, so the run guard is readable before the core edit and fails for the right reason. - -Both guards run both directions, as the view and tool guards do: every value the sentence names is -in its code list, and every value in the code list is named in its sentence or carries a one-line -exemption in the test. Both exemption sets ship empty, since this story states both vocabularies in -full. They exist so the next value added has to make a decision rather than be forgotten, and so -the decision is recorded beside the guard rather than in a commit message. - -Only the run guard lands in this step. The download guard was written here and confirmed to fail -for the right reason, that the core states no such sentence yet, and then moved to the step that -adds the sentence: writing a guard before its prose is the point, but a commit that lands red is -not. Both accessors the guards read are exported for the same reason `StaticViewNames` and -`IdentityColumnNames` already are, since the guards live in a test package that cannot reach -package internals. - ---- - -## Teach the core the Portal report family, the slugs and the recipe - -**Summary**: The substance. One file, one commit, because the family, the slugs and the recipe are a single argument and splitting them would leave the core briefly incoherent. - -**Files affected**: -- `internal/guidance/src/core.md` -- `internal/guidance/guard_test.go` — the slug guard -- `internal/duck/views.go` and `internal/dataset/reporttype.go` — `DimensionSlugs` and - `AthenaReportSlugs`, accessors for the two slug inventories, both of which are unexported today - (`dimensionViews[].slug` and `slugToType`'s keys), so the guard can read either. The second is - named for what it returns rather than `ReportSlugs`, which would overpromise: it is the Athena - half, and the call site unions it with the dimension half. - -**Estimated diff size**: ~180 lines - -Four edits to `core.md`: - -**The run sentence keeps its list and gains the Portal clause** (`core.md:24`). The list is correct for runs and stays as it is; what it lacks is that a Portal run carries no `report_type` and is identified by `execution`. Without that, a model told runs have a `report_type` reaches for it on a Portal run and finds null. - -**The `downloads` entry states the download vocabulary** (`core.md:154`), which is where `portal` and `recovered` belong: both are assigned locally rather than arriving on a run. `recovered` carries the clause that makes it actionable, that a reindex assigns it to a CSV it cannot classify and re-fetching the run restores the real type, which pairs the value with the remedy `RECOVERED_PROVENANCE` already names. Together these two edits turn both guards from the previous step green. - -**The Portal/Athena family and the live-versus-snapshot rule**, in "Runs and their data", phrased as a decision rule because that is what the model needs it for: - -> Reports come in two families. **Athena** reports are computed in the background from the log archive: a run has a query state and its result never changes once it succeeds, so a fresh snapshot means duplicating the run. **Portal** reports are computed from the Portal database on every request, so they list as `live` and a fresh read means re-pulling the same run, not duplicating it. Duplicating a Portal run is refused unless forced. - -The parenthetical "(`report_type` `portal`)" an earlier draft carried is deliberately gone: a Portal run has no `report_type`, which the sentence above this one now says, and repeating the download-side value here would reintroduce the conflation. - -**A `## Report slugs` section**, which is the guarded catalog and the thing that makes `reports_create` usable from the guidance alone: - -```markdown -## Report slugs - -A run is created from a report's slug. These are the ones a data pull starts from. The Portal also -offers aggregate metrics reports that are not listed here; their slugs come from an existing run or -from the researcher guide. - -- `student-id-mapping` — the learners' portal ids and the key that joins them to stored - records, with no names. A run of it is a valid run id for fetching answers, history and - attachments. -- `student-metadata` — the same learners with names and roster labels, joined on `learner_id`. -- `student-answers`, `student-assignment-usage` — per-student Athena reports. -- `student-actions`, `student-actions-with-metadata`, `teacher-actions` — Athena clickstream logs. -``` - -**The recipe**, as a numbered sequence. Verified to parse and to pass the no-commands guard: - -```markdown -### Pulling a cohort's work without authoring an Athena report - -1. Create a `student-id-mapping` run over the learners of interest, assembling the filter - from the available filter options. -2. Fetch that run's answers, history and attachments by its run id. -3. Fetch the run's own report CSV, which becomes `student_id_mapping`. -4. Create and fetch a `student-metadata` run over the same learners, which becomes - `student_metadata`. -5. Materialize the dataset before querying it, when the Materializing a dataset - section says it is worth doing. -6. Query: `answers` joins `student_id_mapping` on `run_remote_endpoint = remote_endpoint`, - and `student_id_mapping` joins `student_metadata` on `learner_id`. See the - `student_id_mapping` entry for what a NULL join key means before filtering on it. - -Re-read any Portal run later by re-pulling the same run id; do not duplicate it. -``` - -The step wording is a constraint, not a style choice: naming the operation rather than the command is what keeps `TestCoreNamesNoCommand` green, and that belongs in a comment beside the guard rather than as folklore. - -Step 5 is a pointer to REPORT-113's rule, carrying the ordering only and no size condition, so there is one place that decides what "large" means. Stage 4 corrected it twice against the landed prose: the drafted wording opened "If the pull is large", which is exactly the condition it was meant to delegate, and it pointed at "when to materialize", which is not what the section is called. REPORT-113 titled it `## Materializing a dataset` (`core.md:189`), and the rule is the bullet beginning "It is worth doing once a dataset is large". Naming the operation rather than the command is still what keeps `TestCoreNamesNoCommand` green. - -Step 6 names the joins because they are the recipe's payoff, but it does not restate the withheld-key rule that governs them. That rule lives on the `student_id_mapping` entry (`core.md:132-137`): a NULL `run_remote_endpoint` is a learner with no secure key, and every such learner carries the same endpoint string, so the key is withheld rather than attributing one learner's answers to all of them. Restating it would put a correctness rule in two places with nothing holding them together; omitting the pointer would let a model follow the recipe and never meet it. Same treatment as the materialize step, and for the same reason. - -The two dimension-view entries gain their two missing facts and nothing else: the slug, and that a run of it drives the `get_*` verbs. Their existing dedup, withheld-key and `hide_names` prose is left untouched. - -**The slug guard**, following the three guards already in the file: - -```go -func TestGuidanceDocumentsOnlyRealSlugs(t *testing.T) { - documented, err := guidance.ParseCatalog(guidance.Core(), "Report slugs") - // every documented slug must exist in code; the reverse is deliberately not checked, - // because the portal offers aggregate reports that have no Go constant today -} -``` - -One direction only, for the reason the requirements give. It needs the union of `dataset.slugToType`'s keys and the dimension views' slugs, neither of which is currently exported, so each gets a small accessor rather than the guard reaching into package internals or a third copy of the list appearing in a test. - -The other half of what the guard means goes beside `slugToType` rather than in the test, because -that is the map that can be wrong and the file a future reader edits when adding a slug. The guard -carries a one-line pointer to it: - -```go -// slugToType is cc-data's own copy of the Athena slugs, not a roster of what the server offers. -// Nothing reconciles it: an unrecognized slug degrades with "unknown to this cc-data version" -// rather than failing, and the guidance guard can only prove the guidance matches this map, never -// that this map matches the server. -``` - -The comment names no ticket, deliberately. The constraint it states is complete without one, and REPORT-130's replacement of this map is planning state rather than something the code cannot express; no other comment in the tree cites a ticket. - -Verified that the union covers the documented set exactly: seven slugs in code, seven documented, none documented that code does not know. So the guard passes on the prose this plan writes, rather than being written and then having the prose trimmed to satisfy it. - ---- - -## Name the re-pull mechanism on each surface - -**Summary**: The per-surface half, which is where the two surfaces are allowed to differ and therefore where a test has to hold them together. - -**Files affected**: -- `internal/guidance/src/skill_header.md` — `--refresh` under "Fetching data" -- `internal/guidance/src/tools.md` — `refresh` on the `get_report` entry, so the MCP guidance carries it -- `internal/mcpserver/tools.go` — six tool descriptions -- `internal/guidance/guard_test.go` — the both-surfaces assertion -- `internal/mcpserver/server_test.go` — the description assertions - -**Estimated diff size**: ~120 lines - -`skill_header.md` gains `--refresh` on the `get report` line, since the core states the rule and cannot state the flag. - -It also gains a `## Making a run` section, which the plan originally missed. The file had no `cc-data reports` subcommand at all, so the recipe's first step, creating a run from a slug and a filter, was unexecutable on the surface that drives the CLI: the model would know `student-id-mapping` and have no verb to use it with. The MCP surface needed nothing, since `tools.md` already names `reports_create` and `reports_filter_options`. A second test, in `package cmd` because it needs the cobra tree, walks every backticked `cc-data ` in both rendered surfaces and asserts it resolves. `root.Find` is not enough on its own: it returns the deepest command it matched plus the args it could not consume, and errors only when the *first* word is unknown, so `reports clone` silently resolves to `reports` with a leftover. The leftovers are the signal, and without checking them the guard passes on a renamed subcommand. - -A test holds the skill surface to it, in the same shape as the re-pull assertion and for the same reason: the core cannot carry a command spelling, so no comparison of the core can notice this file losing one. It asserts nothing about the MCP surface, because the shipped tool guard already fails if either tool leaves `tools.md`, which was checked by removing one. - -`tools.md`'s `get_report` entry gains `refresh` too, and this is not redundant with the tool description. A tool's `Description` is registered with the MCP server and is **not** part of `guidance.Instructions()`, which renders `mcp_header.md` + `core.md` + `tools.md` only. Verified: none of three distinctive description strings appears in the rendered instructions. So the MCP surface's *guidance* learns about `refresh` only if `tools.md` says so, and a both-surfaces test that looked for it in `Instructions()` without this edit would fail on the day it was written. - -Six MCP descriptions gain one named fact each, rather than a general instruction to mention Portal runs: - -| Tool | Fact | -| --- | --- | -| `reports_list` | a run's execution tells an Athena run from a Portal one | -| `reports_filter_options` | it is how a Student ID Mapping run's filter is assembled | -| `get_report` | a Portal report is re-read by passing `refresh`, not by duplicating | -| `get_answers`, `get_history`, `get_attachments` | a Student ID Mapping run id is a valid source | - -Each is asserted in `server_test.go`. These are substring assertions on shipped strings, so the mutation they catch is real: delete the sentence and the test goes red. The drift guard cannot do this job, because it compares tool names and never reads a description. - -The both-surfaces assertion follows `TestBothSurfacesCarryTheAuthRemedy` exactly, and for the same reason: `refresh` is worded differently on each surface, so no name-comparison guard can notice one of them losing it. It compares `Skill()` against `Instructions()`, which is why the `tools.md` edit above is a precondition rather than a nicety. - -It is written only for `refresh`, deliberately. Asserting the core's content on both surfaces would be true by construction and is the decorative test to avoid. - ---- - -## Reconcile the researcher guide - -**Summary**: Last, because it can only be done once the guidance is final. Its expected outcome is a small diff or none. - -**Files affected**: -- `docs/researcher-guide.md` - -**Estimated diff size**: ~40 lines, possibly zero - -REPORT-94 already gave the guide a full Portal-reports treatment (`docs/researcher-guide.md:346-431`), so this is a consistency pass, not a rewrite. Read the finished core against the guide and fix only contradictions or omissions. The known candidate is section 4's "Make a run without the web form", which does not frame the mapping workflow as the way to pull a cohort. - -This step also reconciles REPORT-112's `logs` entry with the two-families framing, which is a core edit rather than a guide one but belongs with the other reconciliation work: the entry names three Athena slugs inline and needs to sit under the family the core now defines. - -The acceptance criterion is that the three documents agree, not that the guide changed. A pass that finds nothing is a passing outcome and should be recorded as one in the PR, rather than becoming a reason to edit prose that is already correct. - -Note the guide's view table is itself guarded (`TestResearcherGuideDocumentsEveryStaticView`), so any view-table edit here is already held by a shipped test. - ---- - -## Open Questions - -None. - -## Self-Review - -Roles: the engineer writing these tests, and the engineer reviewing the commits. Each finding was checked by building the proposed thing far enough to see whether the claim survived. - -### Test author - -#### RESOLVED: The both-surfaces test would have failed on the day it was written - -The plan asserted `refresh` on both surfaces following the auth-remedy precedent, which compares `guidance.Skill()` against `guidance.Instructions()`. But `refresh` was planned to land in `skill_header.md` on one side and in an **MCP tool description** on the other, and a tool `Description` is registered with the server, not rendered into the instructions. Verified: `Instructions()` contains none of three distinctive description strings, and `tools.md`'s `get_report` entry does not mention refresh today. - -So the test would have gone red immediately, and the tempting fix is the wrong one: weakening it to read the registered tools instead would make it pass while no longer holding the thing it exists to hold, which is that the *guidance* on both surfaces carries the rule. Fixed by adding `refresh` to `tools.md`'s `get_report` entry, which is rendered, and keeping the tool description as the separate, separately-asserted surface. - -This is the same class of mistake the requirements-stage review caught in the other direction: assuming a guard reads something it does not. - -#### Checked and not a problem: the slug guard passes on the prose this plan writes - -A guard written against prose that then has to be trimmed to satisfy it is a guard that has been fitted to the answer. Built the union the guard would use: seven slugs in code (five in `dataset.slugToType`, two in `dimensionViews`), seven documented by this plan, and nothing documented that the code does not know. Recorded so the next person does not re-derive it. - -### Commit reviewer - -#### RESOLVED: The section heading and the recipe subheading interact, and the plan did not say so - -`ParseCatalog` closes a section at the next heading of any level, so the `### Pulling a cohort's work` subheading ends the `## Report slugs` section. That is harmless as written, because the slug bullets precede it and the recipe's numbered items would not match the entry pattern anyway. It is a constraint on ordering, though: moving the recipe above the bullets would silently empty the guarded catalog, and `ParseCatalog` only errors when a section documents *no* names, so a partial reorder could shrink it without failing. - -Verified the current arrangement parses all seven slugs with the recipe subheading in place. The plan now carries the ordering constraint next to the section rather than leaving it to be rediscovered. diff --git a/specs/REPORT-95-portal-reports-guidance/requirements.md b/specs/REPORT-95-portal-reports-guidance/requirements.md deleted file mode 100644 index edc8ba3..0000000 --- a/specs/REPORT-95-portal-reports-guidance/requirements.md +++ /dev/null @@ -1,256 +0,0 @@ -# Integrate Portal reports into the Claude skill and MCP guidance - -**Jira**: https://concord-consortium.atlassian.net/browse/REPORT-95 -**Repo**: https://github.com/concord-consortium/cc-data-cli -**Implementation Spec**: [implementation.md](implementation.md) -**Status**: **In Development** - -## Overview - -Teach Claude the Portal-report workflow in the shared guidance both surfaces render, so that a researcher's data question reaches for the right report kind and the whole pull can be driven end to end. Today the guidance documents the two Portal-fed views in detail but never names the report family, never gives the slugs needed to create a run, and never mentions the flag that re-reads a live report. - -## Project Owner Overview - -cc-data can now create Portal report runs, download them, and join them to student answers. Claude cannot reliably drive any of that, because the guidance it reads was written for the Athena-only world and has only been patched where individual stories touched it. The result is a tool whose capabilities have outrun its instructions: a researcher asking Claude for class-level results gets steered down the Athena path, which is slower, needs a report authored first, and returns a frozen snapshot. - -This story closes that gap in the single shared source both the Claude Code skill and the MCP server render, so the two surfaces cannot drift apart on it. - -## Background - -REPORT-104 made one source of guidance rendered into two surfaces (`internal/guidance/guidance.go:27`, `:32`). The split matters here and is the root of several gaps below: - -- `Skill()` renders `skill_header.md` + `core.md`. -- `Instructions()` renders `mcp_header.md` + `core.md` + `tools.md`. - -So anything written in `tools.md` reaches the MCP server only, and the Claude Code skill never sees it. The drift guard compares *names* (views, tools, identity columns) in both directions but cannot notice a concept that reached one surface and not the other. - -**The ticket's background is out of date, and the spec is written against the code instead.** REPORT-95's description says the researcher guide "asserts the opposite in two places" and that both must be reversed. Both strings were removed by REPORT-94 in `564da81`, which replaced them with a full Portal-reports treatment: the Athena/Portal split with `execution` `async` vs `sync`, the `live` state, `get report --refresh`, both Portal report slugs, the `learner_id` join, the hide-names warning, and the one-row-per-learner dedup property (`docs/researcher-guide.md:346-431`). The same story landed the two dimension views and their guidance, which the ticket calls "minimal stubs"; they are not stubs, they are the longest entries in the views catalog. - -That does not leave this story with nothing. It relocates the work: the researcher guide is in good shape and the *guidance* is where the holes are. - -## Verified gaps - -Measured by rendering each surface and probing it, rather than by reading the sources: - -| Fact Claude needs | Skill | MCP instructions | -| --- | --- | --- | -| The phrase "Portal report" | **absent** | present | -| `student-id-mapping` slug | **absent** | **absent** | -| `student-metadata` slug | **absent** | **absent** | -| `--refresh` | **absent** | **absent** | -| `student_id_mapping` / `student_metadata` views | present | present | -| Re-pull rather than duplicate | present | present | - -Four specific defects follow: - -- **The core never says how a Portal run is recognized.** `core.md:24` reads "Report runs have a `report_type` (`answers`, `log`, `usage`)", and for a *run* that list is complete: it is exactly `slugToType`'s value set. A Portal run carries no `report_type` at all, which the wire pins (`portalRunWire` has `"report_type":null`, asserted in `internal/api/portal_download_test.go:52`) and the API type states as design: "report_type is null for a Portal run by design" (`internal/api/types.go:33`). The discriminator is `execution`. So the gap is not a missing enum value, it is that a model told runs have a `report_type` will reach for it on a Portal run, find null, and have nothing to fall back on. `portal` is a value cc-data synthesizes locally from `execution` (`internal/fetch/report.go:162`) and stores on the *download*, which is a different vocabulary in a different place. -- **Neither surface names a report slug.** `reports_create` takes a slug, so the first step of the documented workflow is unexecutable from the guidance alone: Claude has no way to know the string is `student-id-mapping`. The slugs exist in the researcher guide, which the model does not read. -- **Neither surface mentions `--refresh`.** The guidance tells the model to re-read a Portal report rather than duplicating it, and never says how. The flag exists (`cmd/get_report.go:71`), the fetch layer's own error message explains it (`internal/fetch/report.go:144`), and the MCP `get_report` tool already accepts the parameter, but its description is the bare sentence "Download a report CSV into a dataset." -- **The Portal/Athena distinction is MCP-only.** It lives on the `reports_duplicate` entry in `tools.md`, so the skill surface, which is the CLI-driving one, never learns it. - -The MCP descriptions for `reports_list`, `get_report`, `get_answers`, `get_history` and `get_attachments` say nothing about Portal runs or about a Student ID Mapping run being a valid source for the `get_*` verbs. - -## Requirements - -- The shared core names the Portal report family and the Athena/Portal distinction, so both surfaces carry it rather than the MCP surface alone. -- The run sentence at `core.md:24` keeps its list, which is correct for runs, and gains the fact it lacks: a Portal run has no `report_type`, and `execution` is what identifies it. Adding `portal` to that list instead would tell the model a Portal run carries `report_type: portal`, which the wire contradicts, and would undercut the `reports_list` description this story writes to teach exactly the opposite. -- The core carries the live-versus-snapshot model as a decision rule, not a fact: a Portal report is computed per request and is refreshed by re-pulling the same run; an Athena report is a frozen artifact and is refreshed by duplicating it into a new run. -- The guidance names every report slug the code knows: `student-id-mapping` and `student-metadata`, plus the five Athena slugs, so `reports_create` is usable from the guidance alone for any of them. Documenting only the two Portal slugs would leave the guard covering a fraction of the vocabulary while the other five stayed reachable but undocumented. -- The catalog says it is not exhaustive, in one sentence, because it is not. The Portal offers five aggregate metrics reports (`docs/researcher-guide.md:463-468`) whose slugs are in no Go inventory, and REPORT-128 merged three days ago specifically so two of them could be downloaded at all. cc-data never validates a slug before sending it, so they are fully usable and merely undocumented on the surface Claude reads, and there is no discovery path: `ListReports` returns the user's own runs, not the reports the server offers. Without that sentence a model reads seven slugs as the complete set and tells a researcher asking for school metrics that no such report exists. -- What the slug guard proves, and what it cannot, is stated where each half can be acted on. The guard carries its own rule: every documented slug must exist in code, the reverse is not checked, and why. The warning that cc-data's own inventory can be stale sits beside `slugToType`, because that is the map that goes stale and the file someone edits when adding a slug, and the guard points at it. The guard holds the guidance against cc-data's inventories, which catches a typo, since a wrong slug fails at the server with an error that says nothing about spelling. It cannot prove those inventories match the server's: `slugToType` is a static map nothing reconciles, and cc-data already expects it to go stale, warning "report slug %q is unknown to this cc-data version" on an unrecognized one. Green CI must not be read as evidence the slugs are current. -- The recipe names the joins but does not restate the rules that govern them. The withheld-join-key rule already lives on the `student_id_mapping` entry, and a correctness rule stated twice in one file with nothing holding the copies together is how the two drift. The recipe points at it instead, the same treatment the materialize step gets. -- The end-to-end recipe is documented: create a Student ID Mapping run, pull answers, history and attachments by its run id, download the Student ID Mapping and Student Metadata CSVs, materialize first if the pull is large, then query, joining answers and history on `remote_endpoint` and Student Metadata on `learner_id`. -- The recipe's materialize step is a **conditional pointer**, not a restatement: REPORT-113 owns when materializing is worth it, and this step references that rule rather than repeating the condition. The step exists because REPORT-115 templates its CLUE workflow on this recipe and puts materialize in exactly this position ("materialize (REPORT-113) when the history store is large"), for a corpus where it is closest to mandatory. A recipe with no slot for it would force 115 to invent one and the two workflows to diverge structurally. -- The re-pull mechanism is named on both surfaces: `--refresh` for the CLI, the `refresh` parameter for the MCP tool. -- Every `cc-data` command the guidance spells out resolves to a real command, checked across both rendered surfaces. The guidance carries the spellings precisely because the core is forbidden to, so nothing else compares them against the command tree, and a renamed subcommand would leave the skill telling a researcher to run something that is gone. Names only: flags are deliberately not checked, since a flag like `--portal` is required only when no default portal is configured and there is nothing declarative to compare against. -- Each surface carries the verb that acts on a slug, or the recipe's first step is unexecutable there. The MCP surface already had it, since `tools.md` names `reports_create` and `reports_filter_options`; the skill surface named no `cc-data reports` subcommand at all, so a CLI-driving model could read the slug catalog and the recipe and still have no way to create a run or assemble a filter. That is the same defect this story was filed for, one level up: knowing the string and not the command is as unexecutable as not knowing the string. The skill header gains `reports list`, `reports filter-options`, `reports create` and `reports duplicate`, and a test holds the skill surface to it. The lines show only what is unconditionally needed, matching the rest of the file: `--portal` is not a cobra-required flag but a runtime fallback to `default_portal` (`cmd/reports.go:35-43`), so it is named once as a condition rather than repeated on four lines a researcher with a default portal never needs it on. The MCP side needs no new assertion, since the shipped tool guard already fails if either tool leaves the catalog. -- The `student_id_mapping` and `student_metadata` view entries gain exactly the two facts they lack and are otherwise left alone: the slug a run of each is created from, and that such a run's id is a valid source for fetching answers, history and attachments. They already carry the dedup rule, the withheld-join-key rule and the `hide_names` rule. -- The MCP descriptions carry named facts rather than a general instruction to mention Portal runs, and a test asserts each one, since the drift guard checks tool names and never looks at description text: `reports_list` says a run's execution tells Athena from Portal; `reports_filter_options` says it is how a Student ID Mapping run's filter is assembled, since the recipe's first step sends the model straight to it; `get_report` says a Portal report is re-read with `refresh` rather than duplicated; and `get_answers`, `get_history` and `get_attachments` each say a Student ID Mapping run id is a valid source. -- A guard holds that every report slug named in the guidance exists in the code's slug inventories, so a typo cannot ship a slug that fails at the server. -- Each `report_type` vocabulary the guidance states is guarded against the code list that defines it, in both directions. There are two, and conflating them is what made the original requirement wrong: the run sentence is guarded against `slugToType`'s value set, which is what the server sends on a run; the `downloads` entry's vocabulary is guarded against `AllowedReportTypes()`, which is the reports-union allowlist. Every value is named in its own sentence or carries an explicit exemption with a reason. This story exists because guidance drifted from code, and correcting a list by hand would leave the mechanism intact, next to five guards that already do this for views, tools, identity columns, the auth remedy and command spellings. -- The `downloads` view entry states the download vocabulary its `report_type` column carries, which is where `portal` and `recovered` belong: both are values cc-data assigns locally, never values a run arrives with. `recovered` is documented rather than exempted because a reader meets it, on a download: an unclassifiable CSV comes back from a reindex as `"report_type": "recovered"` in `dataset show --json`, as `recovered` in the table's STATUS column, and beside the `RECOVERED_PROVENANCE` warning. It is stated with what produces it and that re-fetching restores the real type, so the value and its remedy arrive together. The core already alludes to such downloads without naming the value (`core.md:151`). -- Each vocabulary gets one readable definition in code, the way the view list already has one. `IsAllowedReportType` is a `switch`, so there is nothing a guard can read; it becomes a slice the switch consults, which is the same move `StaticViewNames()` made and the same reason. The run vocabulary needs no new list, since it is `slugToType`'s value set, but it does need an accessor, as the map is unexported. -- `ParseCatalog`'s identifier pattern is widened to accept hyphens, without which a slug catalog parses as empty and its guard silently checks nothing. The views, tools and identity-column catalogs share that pattern, so a test holds that all three still parse to the same names. -- The `--refresh` spelling is asserted on both surfaces by a test, following the auth remedy, which is the existing precedent for a rule that exists on both surfaces worded differently and that therefore no name-comparison guard can notice. Content placed in `core.md` needs no such test: both surfaces render the core by construction. -- REPORT-112's `logs` view entry is reconciled with the two-families framing this story introduces. It names `student-actions`, `student-actions-with-metadata` and `teacher-actions` inline, which are Athena slugs, so once the core says reports come in two families the entry has to sit visibly under one. This story owns it: it is the capstone that documents the finished surface, and REPORT-112 is already in review. -- The researcher guide is checked against the finished guidance and updated only where it is now wrong or silent. It is not rewritten: REPORT-94 already reversed the two assertions the ticket names. - -## Precondition - -This spec is written ahead of two of its inputs. REPORT-112 adds a `logs` entry to `core.md` and REPORT-113 adds the materialize prose to the same file, so **the branch is rebased and stage 4 re-run before implementation starts**, not after a conflict is discovered. The byte measurements below are pinned to the commit they were taken on and will move; the decision they support does not, since the recipe's cost is absolute and the core only grows. - -Verified against REPORT-112's branch already: with its `logs` entry in place, the widened catalog pattern parses the Views section to the same 12 names before and after, and no hyphenated slug leaks in from the entry's inline `student-actions` references. - -## Technical Notes - -- Command spellings cannot go in the core: `TestCoreNamesNoCommand` rejects any "`cc-data `" string there (`internal/guidance/guard_test.go`). So the *rule* ("refresh a Portal report by re-pulling the same run") goes in the core and the `--refresh` spelling goes in `skill_header.md`, with the MCP surface carrying the parameter on the tool description. This is the same split the auth remedy already uses. -- Two code-side slug inventories exist and do not overlap: `dataset.slugToType` holds the five Athena slugs (`internal/dataset/reporttype.go:16`), and `duck.dimensionViews[].slug` holds `student-id-mapping` and `student-metadata` (`internal/duck/views.go:494`, `:508`). A guard would union them. -- The aggregate Portal metrics reports (Summary Metrics by Assignment and the rest) have slugs that appear in no Go inventory, so a bidirectional slug guard would fail on them. See the open question. The deeper version of the same problem is that the server owns which reports exist and every local copy is a cache with no invalidation, which is why the repo already refused a slug map once: deriving a report's type from `execution` rather than a slug lookup is what lets "a Portal report added to the server later be recognized without a cc-data release" (`internal/fetch/report.go:158-160`). A server-side report catalog that the guidance points at instead of enumerating is the end state; it is filed as REPORT-130 and is out of scope here. -- `ParseCatalog` reads a named section and pulls the backticked identifiers that open each bullet or table row (`internal/guidance/catalog.go:13`), which is the shape any new guarded section has to take. -- The `report_type` guard cannot use `ParseCatalog`, because the sentence that carries the vocabulary opens with prose rather than with a backticked identifier. It does not need to: matching `` `report_type` (...) `` against the rendered core extracts the list from the sentence exactly as written. Verified by running it, which returned `answers`, `log`, `usage` from the current file. So the guard costs no restructuring and no bytes, which matters in a file that has grown 39% since this spec was drafted. -- The rendered skill is currently 12,464 bytes and the MCP instructions 13,462. Both are read on every session, and REPORT-89 is separately trying to protect the model's context budget. - -### Verified: the regex widening is safe, and the guarded section parses - -The slug guard turns on a change to a regex three existing catalogs share, so the claim that widening it changes nothing was run rather than argued. Parsed every catalog before and after widening `` `[a-z0-9_]+` `` to `` `[a-z0-9_-]+` ``: - -| Catalog | Before | After | -| --- | --- | --- | -| core, Views | 11 names | identical | -| core, Identity columns | 4 names | identical | -| tools, Tools | 20 names | identical | -| researcher guide, view table | 11 names | identical | - -The full guidance suite passes with the widening in place. A draft `## Report slugs` section then parsed all seven slugs (`student-id-mapping`, `student-metadata`, `student-answers`, `student-assignment-usage`, `student-actions`, `student-actions-with-metadata`, `teacher-actions`), where before the widening it would have yielded none of the hyphenated ones and reported success anyway. - -### Verified: the proposed core prose survives the no-commands guard - -The recipe is the first procedural content proposed for `core.md`, and `TestCoreNamesNoCommand` rejects any "`cc-data `" spelling there. Drafted the Portal family, the slug catalog and the five-step recipe into the real file and ran the suite: `TestCoreNamesNoCommand` passes, because the steps name the operation ("create a `student-id-mapping` run", "fetch that run's answers") rather than the command. That phrasing is a constraint on the prose, not an accident of it, so the implementation spec should say so; a later edit that helpfully adds the command spelling will fail CI. - -## Out of Scope - -- **Writing the "when to materialize" rule.** REPORT-113's spec assigns that prose to the shared core and the CLI spelling to the skill header, as part of that story. This story references the rule from the recipe and must not restate the condition. -- **CLUE document guidance.** REPORT-115 owns that, and its description names this story's prose as its template. -- Rewriting the researcher guide's Portal-reports treatment, which REPORT-94 landed. -- Any change to the views themselves, or to what `reports_create` accepts. - -## Stage 4 re-run (2026-09-11, rebased onto REPORT-113) - -The branch is now stacked on `REPORT-113-dataset-materialization`, so both inputs the Precondition -names are present: REPORT-112's `logs` entry and REPORT-113's materialize prose are both in -`core.md`. Every assumption above that could have moved was re-run against that state rather than -re-reasoned. Throwaway code, not committed. - -**Holds: `core.md:24` still reads as quoted.** Both stories appended sections rather than editing -near the top, so the `report_type` line has not moved and still omits `portal`. - -**Holds, and is now verified against both inputs: widening `leadingNames` to accept hyphens changes -nothing that parses today.** Measured over the rendered surfaces: Views 12 before and after, Tools -21 before and after, Identity columns 4 before and after, identical name-for-name in each. The -earlier check had only REPORT-112's branch; Tools is 21 because REPORT-113's `dataset_materialize` -entry is now in the catalog, and it parses the same either way. - -**Measured on the finished implementation (2026-09-11):** core 17,002 bytes, skill 20,319, MCP -instructions 20,503. This story added 2,668 to the core, 3,413 to the skill and 2,781 to the -instructions, against a baseline that had itself grown 39% since the spec was drafted. The skill -grows most because it alone carries the command spellings the core is forbidden to name. - -**Moved: every byte measurement, by more than the recipe it was sizing.** Re-measured on this -branch: core 14,334 bytes, skill 16,906, MCP instructions 17,722, against the 10,292 / 12,464 / -13,462 the spec records. The core grew 39%. The decision the numbers support is unchanged and -better supported, since the recipe's +1,532 is 10.7% of the new core against 14.9% of the old, but -the figures in **Technical Notes** and in the recipe decision are pinned to a commit that is two -merges behind and must be re-measured before they are quoted anywhere. - -**Moved, and this one changes the work: the pointer in recipe step 5 has no target.** The step -reads "see when to materialize", which assumed REPORT-113 would leave a rule findable under that -name. It did not: the section is `## Materializing a dataset` (`core.md:189`) and the rule is a -bullet beginning "It is worth doing once a dataset is large and the same questions are being asked -repeatedly". Nothing in the rendered core contains the phrase "when to materialize", so the step as -drafted sends a reader to a heading that does not exist. Step 5 must name the section that does. - -**Moved: step 5 carries the size condition it was supposed to delegate.** The implementation spec -says the step is "deliberately phrased without the size condition so there is one place that -decides what 'large' means", but the drafted step opens "If the pull is large". REPORT-113's bullet -already owns that condition, so the step should carry the ordering only, that materializing comes -before querying, and leave "large" to the core. - -**RESOLVED, and it grew the story: the core's `report_type` list is short by two, not one, and -hand-fixing it would leave the drift mechanism in place.** `IsAllowedReportType` accepts five values (`internal/dataset/reporttype.go:35`): -`answers`, `usage`, `log`, `portal` and `recovered`. The requirement adds `portal` alone, which -leaves the stated vocabulary still not matching the accepted vocabulary, and the requirement's own -justification ("the vocabulary it states is the vocabulary the code accepts") argues for both. -`recovered` is the synthesized type a reindexed CSV with no provenance gets. Verified that a reader -meets it: an aggregate-shaped CSV that `recoverReportType` cannot classify comes back through a -real reindex as `"report_type": "recovered"` in `dataset show --json`, as `recovered` in the -table's STATUS column, and beside a `RECOVERED_PROVENANCE` warning. - -**Resolved by guarding the list rather than by choosing a value.** Correcting three to five by hand -answers today's question and leaves the next report type free to drift, in the one story whose -subject is guidance that drifted. So the vocabulary gets a bidirectional guard like views, tools -and identity columns already have, the code gets one definition of the vocabulary for the guard to -read, and the question becomes structural: a type is documented or it is exempted with a reason, -and neither can be skipped. `recovered` is then documented, because it is reader-facing; an -exemption would have been defensible only for a value nobody sees. - -## Open Questions - -### RESOLVED: Should the slug guard be bidirectional, and what happens to the aggregate reports? - -**Context**: A guard that every guidance-named slug exists in code would have caught the fact that no slug is documented at all. But the two code inventories cover only seven slugs, and the Portal aggregate reports (Summary Metrics by Assignment, Detailed Metrics by Assignment, Teacher Status, Detailed Metrics by School, Summary Metrics by Subject Area) exist in the researcher guide and in the server, with no Go constant anywhere. A guard demanding that every documented slug exist in code would fail on those the moment anyone documents them. - -**Options considered**: -- A) One direction only: every slug the guidance names must exist in a code inventory. Catches the typo that matters, since a wrong slug fails at the server, and stays silent about slugs the code has no opinion on. -- B) Bidirectional, with the aggregate slugs added to a Go inventory first so both sides can agree. More complete, and it makes the code the roster of known reports, but it adds a list that must track the server. -- C) No guard. The slugs are few and the researcher guide already lists them. - -**Decision**: A, one direction. A wrong slug fails at the server with an error that says nothing about spelling, and that is the failure worth catching; the code having no constant for a report the server offers is not a defect the guidance should be blocked on. B would make the Go inventory a roster of every report the server exposes, which is a second thing to keep in sync with a system that changes without us. - -**But the guard does not work as assumed, and the discovery changes the work.** `ParseCatalog`'s `leadingNames` regex matches `` `[a-z0-9_]+` `` (`internal/guidance/catalog.go:13`), which excludes hyphens. Ran it against a slug section: `student-id-mapping` and `student-metadata` were both **silently skipped**, and the call returned success because one underscored control entry was present. A guard built on it as-is would pass while checking nothing, which is the exact shape of a test that cannot fail. - -So the guard requires widening the character class to accept hyphens. That regex is shared by the views, tools and identity-column catalogs, so the change needs a test proving those three still parse to identical name lists. No existing documented name contains a hyphen, so the widening cannot change their results, but that is an argument for writing the test rather than for skipping it. - -### RESOLVED: How much of the recipe belongs in the core, given the context budget? - -**Context**: The core is rendered into every session on both surfaces, and is already 12.4 KB. A full end-to-end recipe with the joins spelled out is perhaps 400 to 600 bytes more, on top of the Portal family, the slugs and the live-versus-snapshot rule. REPORT-89 is separately concerned with what the model has to carry. The alternative is a compressed decision rule in the core plus the worked recipe in the researcher guide, which the human reads and the model does not. - -**Options considered**: -- A) Full recipe in the core. The model can execute the workflow without the human relaying steps, which is the story's stated point. -- B) Decision rule in the core (which report kind, and that a mapping run's id drives the `get_*` verbs), worked example in the researcher guide only. Smallest context cost; relies on the model composing the steps from the view entries it already has. -- C) Full recipe in the core now, and let REPORT-89's cold walk-through decide whether it earns its bytes once the whole surface is measurable. - -**Decision**: A, and the measurement is what decides it. Both versions were drafted and measured against the real file: the minimal decision rule is +765 bytes on a 10,292-byte core, the full recipe is +1,532, so **the recipe itself costs 767 bytes**, roughly 200 tokens, and takes the rendered skill from 12,464 to 13,996. - -That is not where a context budget is won or lost, and the story exists precisely so the model can execute the workflow rather than narrate it. B would save 767 bytes by relying on the model to compose the sequence from view entries that document the joins but never say a mapping run's id drives the `get_*` verbs, which is the one fact it cannot infer. - -A and C are compatible rather than alternatives: write it now, and REPORT-89's cold walk-through can trim it with the whole surface in view, which is a better place to judge it from than here. - -### RESOLVED: Does this story still own researcher-guide work? - -**Context**: The ticket's guide requirement was to reverse two assertions, which REPORT-94 already did, and the guide's Portal treatment is now the most complete of the three documents. The remaining candidates are small: it does not mention `reports create` in section 4's "Make a run without the web form" in terms of the mapping workflow, and it will need whatever the guidance decides about slugs to stay consistent. - -**Options considered**: -- A) Yes, narrowed to a consistency pass: after the guidance is written, check the guide against it and fix only contradictions or omissions, with the acceptance criterion being that the three documents agree. -- B) No. Drop the guide from this story and note in the ticket that REPORT-94 discharged it. - -**Decision**: A, narrowed to a consistency pass. The guide is the only one of the three documents a human reads end to end, and this story is about to add slugs and a workflow to the other two; leaving it out would let the three drift on their first change. The pass is cheap because the guide is already correct: it looks for contradictions and omissions against the finished guidance, and changes nothing else. - -The acceptance criterion is that the three documents agree, not that the guide was edited. A pass that finds nothing to change is a passing outcome, and it should be recorded as one rather than treated as a reason to edit something. - -## Self-Review - -Roles: Senior Engineer, QA Engineer, Technical Writer. Findings that did not survive a check against the code are not recorded. - -### Senior Engineer - -#### RESOLVED: "Enrich the stubs" named no actual gap - -The requirement inherited the ticket's framing that the two dimension-view entries are minimal stubs to be enriched, which is both wrong and unactionable: they are the longest entries in the views catalog. Read them against the workflow and the gap is exactly two facts, neither of which is inferable from what is there: the slug a run is created from, and that such a run's id is a valid source for the `get_*` verbs. Everything else the workflow needs, the joins included, is already written. - -Fixed by naming the two facts. An instruction to "enrich" would have invited a rewrite of prose that is already correct, which is how a documentation change becomes a merge conflict for no gain. - -#### RESOLVED: The byte baseline is measured on a commit two unlanded stories both change - -The spec records the rendered surfaces at 12,464 and 13,462 bytes and uses the difference between a minimal and a full recipe to decide the context question. Checked what is in flight: REPORT-112's branch adds **32 lines to `core.md`** for the logs view, and REPORT-113's spec adds the materialize prose to the same file. So the baseline is stale before this is implemented, and `core.md` is very likely to conflict on merge. - -The decision it supports does not move: 767 bytes stays 767 bytes whatever the denominator, and the ratio only shrinks as the core grows. But the number is now pinned to the commit it was measured on, and the stage-4 re-run when 112 and 113 land is recorded as expected work rather than a surprise, along with the conflict. - -### QA Engineer - -#### RESOLVED: "Mention Portal runs where relevant" is a requirement that cannot fail - -The drift guard compares tool *names* in both directions and never reads a description (`internal/guidance/guard_test.go`). So a requirement phrased as "the descriptions mention Portal runs where relevant" has no way to be checked and no way to be wrong: any description satisfies it under a generous reading, and no test would go red if every description were left untouched. - -Fixed by naming the specific fact each of the five descriptions must carry and asserting each with a test. That is a test with a mutation to catch: delete the sentence and it goes red. - -#### RESOLVED: The one rule that needs a both-surfaces test was not distinguished from the ones that do not - -The spec asked for `--refresh` to be named on both surfaces without saying how that would be held, and asked for the Portal family to be in the core in the same breath, as though both needed the same protection. They do not, and conflating them would have produced either a redundant test or a missing one. - -Content in `core.md` is rendered by both surfaces by construction, so a both-surfaces assertion on it is true no matter what and is exactly the decorative test to avoid. The `--refresh` spelling is the opposite case: it lives in `skill_header.md` on one surface and in a tool description on the other, worded differently, which is precisely the shape `TestBothSurfacesCarryTheAuthRemedy` exists for. That precedent is now cited as the pattern to follow. - -### Technical Writer - -#### RESOLVED: The story had no stated success condition for the document it does not change - -The researcher-guide requirement said the guide is "checked and updated only where it is now wrong or silent", which leaves a reviewer unable to tell a completed pass from a skipped one. The resolution now states that the acceptance criterion is the three documents agreeing, and that a pass finding nothing to change is a passing outcome to be recorded rather than a prompt to edit something. From 71f6be0eeaf37e0f6314205d7c268461e6761a02 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 10:20:22 -0400 Subject: [PATCH 11/12] fix: hold every slug the core names, not just the catalog's [REPORT-95] The slug guard parsed the `## Report slugs` catalog, so a typo in the slugs the view entries and the recipe name passed unnoticed, which is weaker than the guard's own claim. It now scans the rendered core for the backticked lowercase-hyphenated shape, which today matches the seven slugs and nothing else, with an empty exemption map for a future hyphenated identifier that is not a slug. Two prose corrections come with it. The researcher guide gave a Portal run `report_type` `portal`, which is the type cc-data assigns to the download it records; the run itself carries none, and the core now teaches that `execution` is the discriminator. The core also sent readers to the guide for the aggregate reports' slugs, which the guide does not list, so it now says cc-data cannot enumerate them and points at an existing run instead. --- docs/researcher-guide.md | 7 ++++--- internal/guidance/guard_test.go | 23 +++++++++++++++++++--- internal/guidance/src/core.md | 4 ++-- specs/REPORT-95-portal-reports-guidance.md | 6 +++--- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/docs/researcher-guide.md b/docs/researcher-guide.md index cdb588f..bdcf3f9 100644 --- a/docs/researcher-guide.md +++ b/docs/researcher-guide.md @@ -398,9 +398,10 @@ and queryable through the same `reports` view. `cc-data` records each run's type Reports come in two flavors. **Athena reports** (`execution` `async`) are computed in the background from the log archive, so a run has a query state and its result never changes once it succeeds. **Portal reports** (`execution` -`sync`, `report_type` `portal`) are computed from the Portal database every time -you ask for them, so `reports list` shows their state as `live` and re-pulling -one with `get report --refresh` is how you get current data. +`sync`) are computed from the Portal database every time you ask for them, so +`reports list` shows their state as `live` and re-pulling one with +`get report --refresh` is how you get current data. Such a run carries no +`report_type` of its own; `cc-data` labels the download it records `portal`. **Athena student data, one row per student:** diff --git a/internal/guidance/guard_test.go b/internal/guidance/guard_test.go index 7c013dc..1ce51fe 100644 --- a/internal/guidance/guard_test.go +++ b/internal/guidance/guard_test.go @@ -199,16 +199,33 @@ func TestGuidanceStatesTheDownloadReportTypes(t *testing.T) { dataset.AllowedReportTypes(), downloadReportTypeExemptions, "download") } +// slugShaped matches a backticked lowercase-hyphenated identifier. Every such name in +// the core is a report slug, so scanning for the shape covers the slugs the prose and +// the recipe name as well as the ones the catalog lists. slugExemptions is where a +// hyphenated identifier that is not a slug has to declare itself. Only the core is +// scanned: `cc-data` takes the same shape, and it appears on the other surfaces. +var slugShaped = regexp.MustCompile("`([a-z0-9]+(?:-[a-z0-9]+)+)`") + +var slugExemptions = map[string]bool{} + // TestGuidanceDocumentsOnlyRealSlugs runs one direction only. The reverse is // deliberately not checked, because the portal offers aggregate reports that have // no Go constant today. What this cannot prove is recorded beside slugToType. func TestGuidanceDocumentsOnlyRealSlugs(t *testing.T) { - documented, err := guidance.ParseCatalog(guidance.Core(), "Report slugs") - if err != nil { + if _, err := guidance.ParseCatalog(guidance.Core(), "Report slugs"); err != nil { t.Fatal(err) } + var named []string + for _, m := range slugShaped.FindAllStringSubmatch(guidance.Core(), -1) { + if !slugExemptions[m[1]] { + named = append(named, m[1]) + } + } + if len(named) == 0 { + t.Fatal("the core names no slug, so this checks nothing") + } inCode := append(dataset.AthenaReportSlugs(), duck.DimensionSlugs()...) - if m := guidance.Missing(documented, inCode); len(m) > 0 { + if m := guidance.Missing(named, inCode); len(m) > 0 { t.Fatalf("guidance names slugs the code does not know: %v", m) } } diff --git a/internal/guidance/src/core.md b/internal/guidance/src/core.md index 52a0fb7..3025e4e 100644 --- a/internal/guidance/src/core.md +++ b/internal/guidance/src/core.md @@ -185,8 +185,8 @@ cross-portal identity is out of scope. ## Report slugs A run is created from a report's slug. These are the ones a data pull starts -from. The Portal also offers aggregate metrics reports that are not listed here; -their slugs come from an existing run or from the researcher guide. +from. The Portal also offers aggregate metrics reports whose slugs cc-data +cannot enumerate; take one from a run that already exists. - `student-id-mapping` — the learners' portal ids and the key that joins them to stored records, with no names. A run of it is a valid run id for fetching diff --git a/specs/REPORT-95-portal-reports-guidance.md b/specs/REPORT-95-portal-reports-guidance.md index cff7111..5de729d 100644 --- a/specs/REPORT-95-portal-reports-guidance.md +++ b/specs/REPORT-95-portal-reports-guidance.md @@ -41,7 +41,7 @@ Measured by rendering each surface and probing it, rather than by reading the so - Each surface carries the verb that acts on a slug, or the recipe's first step is unexecutable there. - The `student_id_mapping` and `student_metadata` view entries gain exactly the two facts they lack, the slug and that such a run's id drives the `get_*` verbs, and are otherwise left alone. - The MCP descriptions carry named facts rather than a general instruction to mention Portal runs, and a test asserts each one. -- A guard holds that every report slug named in the guidance exists in the code's slug inventories. +- A guard holds that every report slug named in the guidance exists in the code's slug inventories. It scans the whole rendered core for the slug shape rather than the catalog alone, since the prose and the recipe name slugs the catalog listing would not cover. - Each `report_type` vocabulary the guidance states is guarded against the code list that defines it, in both directions. There are two, and conflating them is what made the original requirement wrong. - The `downloads` view entry states the download vocabulary its `report_type` column carries, which is where `portal` and `recovered` belong. - Each vocabulary gets one readable definition in code, the way the view list already has one. @@ -58,7 +58,7 @@ Measured by rendering each surface and probing it, rather than by reading the so - The `report_type` guards cannot use `ParseCatalog`, because the sentences carrying the vocabularies open with prose rather than a backticked identifier. Matching `` `report_type` (...) `` against the rendered core extracts each list from its sentence as written, so the guards cost no restructuring and no bytes. The core is hard-wrapped, so the match has to tolerate a list spanning lines. - A tool's `Description` is registered with the MCP server and is **not** part of `guidance.Instructions()`, which renders `mcp_header.md` + `core.md` + `tools.md` only. So the MCP surface's *guidance* learns about `refresh` only if `tools.md` says so. - `cobra.Find` returns the deepest command it matched plus the args it could not consume, and errors only when the first word is unknown, so a wrong subcommand resolves to its parent with leftovers. The leftovers are the signal the command guard keys on. -- Measured on the finished implementation: core 17,018 bytes, skill 20,306, MCP instructions 20,519. This story added 2,684 / 3,400 / 2,797 against a baseline that had itself grown 39% since the spec was drafted. The skill grows most because it alone carries the command spellings the core is forbidden to name. +- Measured on the finished implementation: core 17,003 bytes, skill 20,291, MCP instructions 20,504. This story added 2,669 / 3,385 / 2,782 against a baseline that had itself grown 39% since the spec was drafted. The skill grows most because it alone carries the command spellings the core is forbidden to name. ## Out of Scope @@ -164,4 +164,4 @@ Measured by rendering each surface and probing it, rather than by reading the so - **"Mention Portal runs where relevant" was a requirement that could not fail.** The drift guard compares tool *names* and never reads a description. Fixed by naming the specific fact each description must carry and asserting each, which is a test with a mutation to catch. - **A both-surfaces test on core content is decorative.** The core is rendered by both surfaces by construction, so asserting it on each is true no matter what. Only the per-surface halves, the `--refresh` spelling and the `refresh` parameter, need holding together. - **The both-surfaces test would have failed on the day it was written.** `refresh` was planned for `skill_header.md` and the tool description only, and a tool's description is not part of the rendered instructions. The tempting fix, reading the registered tools instead, would have made it pass while no longer holding the thing it exists for. `tools.md` gains the parameter instead, which is a precondition rather than a nicety. -- **The four shipped catalogs need no new test for the regex widening.** The view, tool, identity-column and researcher-guide guards already compare each catalog against its code-derived set in both directions, so a name the widened pattern gained or lost fails one of them. A new test asserting the same comparison could not fail unless one of those four also failed. +- **The regex widening gets a parser test, not a catalog test.** `TestParseCatalogReadsHyphenatedNames` pins what the pattern now accepts. What was deliberately not added is a fifth guard re-comparing the shipped catalogs: the view, tool, identity-column and researcher-guide guards already compare each against its code-derived set in both directions, so a name the widened pattern gained or lost fails one of them, and a new test asserting the same comparison could not fail unless one of those four also failed. From 169fe604fe6f15c99fdc1be09fb0711a65fa2970 Mon Sep 17 00:00:00 2001 From: Doug Martin Date: Fri, 11 Sep 2026 14:41:16 -0400 Subject: [PATCH 12/12] docs: label the Portal slugs and say the run-id fact once [REPORT-95] The catalog's two Portal entries now carry "Portal, live" beside the "Athena" marks on their neighbors, and the sentence saying a run of one is a valid run id for answers, history and attachments moves to the intro so it covers both instead of reading as a difference between them. The researcher guide's matching pair gets the same move. The command guard's failure messages name the guidance rather than the skill, since it scans both surfaces. RunReportTypes's comment says what it returns, the distinct types of the known slug map, rather than claiming to be the server's vocabulary. The two view bullets that were left ragged by an insert are re-wrapped. --- cmd/guidance_commands_test.go | 4 +-- docs/researcher-guide.md | 8 ++--- internal/dataset/reporttype.go | 5 ++-- internal/guidance/src/core.md | 53 +++++++++++++++++----------------- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/cmd/guidance_commands_test.go b/cmd/guidance_commands_test.go index 7bca0fd..366fd95 100644 --- a/cmd/guidance_commands_test.go +++ b/cmd/guidance_commands_test.go @@ -37,11 +37,11 @@ func TestGuidanceNamesOnlyRealCommands(t *testing.T) { // signal. found, rest, err := root.Find(words) if err != nil { - t.Errorf("the skill names `cc-data %s`, which is not a command: %v", strings.Join(words, " "), err) + t.Errorf("the guidance names `cc-data %s`, which is not a command: %v", strings.Join(words, " "), err) continue } if len(rest) > 0 { - t.Errorf("the skill names `cc-data %s`, but %q is not a subcommand of %q", + t.Errorf("the guidance names `cc-data %s`, but %q is not a subcommand of %q", strings.Join(words, " "), rest[0], found.CommandPath()) } } diff --git a/docs/researcher-guide.md b/docs/researcher-guide.md index 405fedd..3c0d112 100644 --- a/docs/researcher-guide.md +++ b/docs/researcher-guide.md @@ -433,14 +433,14 @@ its result never changes once it succeeds. **Portal reports** (`execution` The three `log` reports share the same clickstream columns; `student-answers` and `student-assignment-usage` share the same per-student, per-resource shape. -**Portal reports, computed live.** Two of them name a set of learners, and are -what you use to pull those learners' answers, history and attachments without +**Portal reports, computed live.** Two of them name a set of learners, and a run +of either is a valid run id for `get answers`, `get history` and +`get attachments`, so they are what you use to pull those learners' work without authoring an Athena report first: - **Student ID Mapping** (slug `student-id-mapping`): the portal ids and the `run_remote_endpoint` that joins each learner to their stored records, and no - names. A run of this report is a valid run id for `get answers`, `get history` - and `get attachments`. It also becomes the `student_id_mapping` view. + names. It becomes the `student_id_mapping` view. - **Student Metadata** (slug `student-metadata`): the human-readable context (name, username, class, school, teachers, permission forms), joining to Student ID Mapping on `learner_id`. Names are hidden unless you are an admin diff --git a/internal/dataset/reporttype.go b/internal/dataset/reporttype.go index 211ae6b..5750d3f 100644 --- a/internal/dataset/reporttype.go +++ b/internal/dataset/reporttype.go @@ -47,8 +47,9 @@ func AllowedReportTypes() []string { return append([]string(nil), allowedReportTypes...) } -// RunReportTypes returns what the server sends as a run's report_type. A Portal run sends -// none, which is why execution and not this list identifies one. +// RunReportTypes returns the distinct report types in cc-data's known Athena slug map. It +// does not enumerate or validate the server's vocabulary. A Portal run carries no +// report_type at all, which is why execution and not this list identifies one. func RunReportTypes() []string { seen := map[string]bool{} var out []string diff --git a/internal/guidance/src/core.md b/internal/guidance/src/core.md index 3025e4e..a77a57c 100644 --- a/internal/guidance/src/core.md +++ b/internal/guidance/src/core.md @@ -139,28 +139,27 @@ like `wildfire_2026.answers`): - `student_id_mapping` — one row per `learner_id` from Student ID Mapping runs (slug `student-id-mapping`), deduplicated across runs with the latest fetch winning. Such a run's id is a valid source for fetching answers, history and - attachments. Join to `answers` and - `history` on `run_remote_endpoint = remote_endpoint`. A NULL - `run_remote_endpoint` is a learner with no secure key, not missing data: every - such learner carries the same endpoint string, so the join key is withheld - rather than attributing one learner's answers to all of them. To check whether - one run repeated a learner, compare that run's own row count with its distinct - learner count: `SELECT count(*), count(DISTINCT learner_id) FROM - report_`. Do not compare against this view's rows for that run: it - deduplicates **across** runs, so a learner a later run also holds is absent - here without the earlier run having repeated anything. + attachments. Join to `answers` and `history` on `run_remote_endpoint = + remote_endpoint`. A NULL `run_remote_endpoint` is a learner with no secure + key, not missing data: every such learner carries the same endpoint string, so + the join key is withheld rather than attributing one learner's answers to all + of them. To check whether one run repeated a learner, compare that run's own + row count with its distinct learner count: `SELECT count(*), count(DISTINCT + learner_id) FROM report_`. Do not compare against this view's rows for + that run: it deduplicates **across** runs, so a learner a later run also holds + is absent here without the earlier run having repeated anything. - `student_metadata` — one row per `learner_id` from Student Metadata runs (slug `student-metadata`), same dedupe and the same `run_remote_endpoint` rule, - carrying the names and roster labels the mapping view deliberately has none of. - Such a run's id is a valid source for fetching answers, history and attachments. Join to - `student_id_mapping` on `learner_id`. `hide_names` is the run's own setting: - where it is true, `student_name` holds the student id and `username` a hash, - so a dataset holding runs fetched under different roles is filterable rather - than silently mixed. It is NULL for any download whose filter was not - recorded, which includes every download made before cc-data recorded filters - and any recovered by a reindex with no manifest. The same rows also reach - `reports`, which has no such column, so name-sensitive work belongs on this - view or on a type-qualified `downloads` join. + carrying the names and roster labels the mapping view deliberately has none + of. Such a run's id is a valid source for fetching answers, history and + attachments. Join to `student_id_mapping` on `learner_id`. `hide_names` is the + run's own setting: where it is true, `student_name` holds the student id and + `username` a hash, so a dataset holding runs fetched under different roles is + filterable rather than silently mixed. It is NULL for any download whose + filter was not recorded, which includes every download made before cc-data + recorded filters and any recovered by a reindex with no manifest. The same + rows also reach `reports`, which has no such column, so name-sensitive work + belongs on this view or on a type-qualified `downloads` join. - `downloads` — a manifest dimension table: `run_id`, `type`, `slug`, `report_type`, `hide_names` and `complete`. A download's `report_type` (`answers`, `log`, `usage`, `portal`, `recovered`) is cc-data's own, not the @@ -185,14 +184,14 @@ cross-portal identity is out of scope. ## Report slugs A run is created from a report's slug. These are the ones a data pull starts -from. The Portal also offers aggregate metrics reports whose slugs cc-data -cannot enumerate; take one from a run that already exists. +from. A run of either Portal report is a valid run id for fetching answers, +history and attachments. The Portal also offers aggregate metrics reports whose +slugs cc-data cannot enumerate; take one from a run that already exists. -- `student-id-mapping` — the learners' portal ids and the key that joins them to - stored records, with no names. A run of it is a valid run id for fetching - answers, history and attachments. -- `student-metadata` — the same learners with names and roster labels, joined on - `learner_id`. +- `student-id-mapping` — Portal, live: the learners' portal ids and the key that + joins them to stored records, with no names. +- `student-metadata` — Portal, live: the same learners with names and roster + labels, joined on `learner_id`. - `student-answers`, `student-assignment-usage` — per-student Athena reports. - `student-actions`, `student-actions-with-metadata`, `teacher-actions` — Athena clickstream logs.