diff --git a/cmd/guidance_commands_test.go b/cmd/guidance_commands_test.go new file mode 100644 index 0000000..366fd95 --- /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 guidance names `cc-data %s`, which is not a command: %v", strings.Join(words, " "), err) + continue + } + if len(rest) > 0 { + t.Errorf("the guidance 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/docs/researcher-guide.md b/docs/researcher-guide.md index 0c7af75..3c0d112 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 Report types section describes that path. ### A complete session @@ -396,9 +400,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:** @@ -428,14 +433,14 @@ one with `get report --refresh` is how you get current data. 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 8a0f48b..5750d3f 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,10 @@ 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. var slugToType = map[string]string{ "student-answers": ReportTypeAnswers, "student-assignment-usage": ReportTypeUsage, @@ -28,12 +34,52 @@ 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 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 + 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 } + +// 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) + } + sort.Strings(out) + return out +} diff --git a/internal/duck/views.go b/internal/duck/views.go index f7e85bc..51db436 100644 --- a/internal/duck/views.go +++ b/internal/duck/views.go @@ -943,3 +943,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/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/internal/guidance/guard_test.go b/internal/guidance/guard_test.go index 78eae6f..1ce51fe 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,117 @@ 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, 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) + "\\s+`report_type`\\s*\\(((?:`[a-z]+`(?:,\\s*)?)+)\\)") + 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 +} + +// 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"), + 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) + } +} + +func TestGuidanceStatesTheDownloadReportTypes(t *testing.T) { + assertVocabulary(t, reportTypesIn(t, guidance.Core(), "A download's"), + 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) { + 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(named, inCode); len(m) > 0 { + 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. +// 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") + } + if !strings.Contains(guidance.Instructions(), "passing `refresh`") { + t.Error("instructions: the refresh parameter is missing") + } +} diff --git a/internal/guidance/src/core.md b/internal/guidance/src/core.md index 1e3244f..a77a57c 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. @@ -77,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 @@ -129,30 +136,36 @@ 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 - `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, 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_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. +- `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. +- `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. - `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 +181,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. 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` — 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. + +### 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. + ## Identity columns A record is identified across the stores by these columns, which are also the diff --git a/internal/guidance/src/skill_header.md b/internal/guidance/src/skill_header.md index 8f8d019..0f974d4 100644 --- a/internal/guidance/src/skill_header.md +++ b/internal/guidance/src/skill_header.md @@ -38,9 +38,24 @@ 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` — 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 -- `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 a49a259..dac86fa 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 43da7a5..073673a 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 d501b95..5c0a692 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 { diff --git a/specs/REPORT-95-portal-reports-guidance.md b/specs/REPORT-95-portal-reports-guidance.md new file mode 100644 index 0000000..5de729d --- /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. 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. +- `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,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 + +- **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 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.