Integrate Portal reports into the Claude skill and MCP guidance [REPORT-95]

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))
+}

docs/researcher-guide.md

@@ -243,8 +243,12 @@ cc-data reports create --report-slug student-answers --report-filter '{"cohort":
 
 Use `--report-filter-file <path>` when the filter is too long to quote. To take a
 fresh snapshot of a run you already have, `cc-data reports duplicate <run-id>`.
-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

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
+}

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
+}

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

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)
+	}
+}

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")
+	}
+}