Repository navigation
Integrate Portal reports into the Claude skill and MCP guidance [REPORT-95] - #17
Conversation
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.
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.
…T-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.
… [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.
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.
…EPORT-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.
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.
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.
…RT-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.
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.
There was a problem hiding this comment.
🟡 Changes recommended
Guidance inconsistencies and incomplete slug validation can still mislead users or permit documentation drift.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Portal-report workflows to shared Claude skill and MCP guidance, backed by drift guards.
Changes:
- Documents Portal/Athena behavior, report slugs, refresh semantics, and cohort workflow.
- Expands MCP tool descriptions and researcher guidance.
- Adds guards for slugs, report types, commands, and descriptions.
File summaries
| File | Description |
|---|---|
specs/REPORT-95-portal-reports-guidance.md |
Records requirements and decisions. |
internal/mcpserver/tools.go |
Adds Portal facts to tool descriptions. |
internal/mcpserver/server_test.go |
Guards tool-description facts. |
internal/guidance/src/tools.md |
Documents MCP refresh behavior. |
internal/guidance/src/skill_header.md |
Adds report CLI workflow. |
internal/guidance/src/core.md |
Adds shared Portal guidance and recipe. |
internal/guidance/guard_test.go |
Adds vocabulary and slug guards. |
internal/guidance/catalog.go |
Supports hyphenated catalog names. |
internal/guidance/catalog_test.go |
Tests hyphenated parsing. |
internal/duck/views.go |
Exposes dimension-view slugs. |
internal/dataset/reporttype.go |
Exposes report-type and slug inventories. |
docs/researcher-guide.md |
Adds refresh and cohort guidance. |
cmd/guidance_commands_test.go |
Validates documented command names. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
There was a problem hiding this comment.
Looks good 👍 A Claude review raised the minor issues below that you may want to consider addressing before merge.
1. Label the two Portal slugs. internal/guidance/src/core.md:191-198 marks two entries "Athena" and leaves student-id-mapping and student-metadata unmarked. A reader must infer their family from the recipe's last line. Add "Portal, live" to both.
2. One bullet carries a sentence its neighbour lacks. In the same list, student-id-mapping says a run of it is a valid run id for fetching answers, history and attachments; student-metadata says nothing. Between two entries of the same shape, that reads as a difference. Say it on both, or on neither.
3. The command guard blames the wrong file. cmd/guidance_commands_test.go:41-48 scans the skill and the MCP instructions, but every message says "the skill names". Change it to "the guidance names".
4. RunReportTypes's comment claims too much. internal/dataset/reporttype.go:50-52 says it returns what the server sends. It returns the distinct values of slugToType. Those match today; they are not the same thing. Suggested wording: "RunReportTypes returns the distinct report types in cc-data's known Athena slug map. It does not enumerate or validate the server's vocabulary."
5. Re-wrap one paragraph. internal/guidance/src/core.md:155 is 90 columns and line 142 is left short, so the text was not re-wrapped after the insert.
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.
|
Thanks. All five addressed in 169fe60.
|
cc-data has been able to create Portal report runs, download them and join them to student answers since REPORT-94, but 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 happened to touch 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.
The gap, measured rather than assumed
Each surface was rendered and probed, rather than read:
student-id-mappingslugstudent-metadataslug--refreshstudent_id_mapping/student_metadataviewsThe ticket's own background turned out to be stale, so the spec was written against the code instead. It said the researcher guide "asserts the opposite in two places" and that the two dimension views are "minimal stubs". REPORT-94 had already removed both strings in
564da81and landed the two views, which are 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.What this adds
The shared core (rendered by both surfaces) learns the Athena/Portal distinction, the live-versus-snapshot model as a decision rule rather than a fact, the seven report slugs the code knows, and a six-step recipe for pulling a cohort's work without authoring an Athena report. The recipe's materialize step is a pointer, not a restatement: REPORT-113 owns that rule, and REPORT-115 templates its CLUE workflow on this recipe.
Command spellings cannot go in the core, because
TestCoreNamesNoCommandrejects any "cc-data" string there, so the rule goes in the core and the spelling goes inskill_header.md, with the MCP surface carrying the parameter ontools.md. That is the same split the auth remedy already uses. A tool'sDescriptionis registered with the server and is not part ofguidance.Instructions(), so a fact placed only in a description does not reach the MCP guidance at all;tools.mdgainsrefreshfor that reason.Four new guards, and what each one catches
dataset.slugToTypeorduck.dimensionViews[].slug. The whole rendered core is scanned for the slug shape, not just the catalog section, so a typo in the view entries or the recipe is caught too. One direction only, deliberately: a slug the server offers and this repo has no constant for is not a defect the guidance should be blocked on.report_typevocabularies, both directions. There are two of them, and conflating them is what made the original requirement wrong. A run carries[answers log usage]and nothing else, because a Portal run'sreport_typeis null on the wire; the reports union additionally admitsportalandrecovered, both assigned locally. Addingportalto a sentence about runs would have told the model a Portal run carriesreport_type: portalwhile this story's ownreports_listdescription teaches thatexecutionis the discriminator. The gap was never a missing enum value, it was a missing discriminator.cc-data ...the guidance spells out resolves to a real command, across both rendered surfaces. Names only.--portalis not a cobra-required flag but a runtime fallback todefault_portal, so there is nothing declarative to compare a flag against. The guard has to checkcobra.Find's leftover args rather than only its error, or a renamed subcommand resolves to its parent and the guard passes; two of three mutations survived until that was fixed.ParseCatalog's identifier pattern had to be widened to accept hyphens first. Without that, a slug catalog parses as empty and its guard reports success while checking nothing. The four shipped catalogs parse identically before and after, and the existing bidirectional guards would fail on any name the widened pattern gained or lost, so no new test was added for them.Deliberately not done
The Portal offers five aggregate metrics reports whose slugs are in no Go inventory and which no endpoint enumerates, so three of the five are not in this repo to copy. The catalog says it is not exhaustive, in one sentence, and REPORT-130 is filed to expose the v1 report catalog from the server so nothing has to hardcode slugs at all.
Tree.api_report_slugs/0already computes exactly that list server-side and already gates report creation.slugToTypegains a comment saying what it is and what nothing reconciles it against. The repo already refused a slug map once for this reason: deriving a report's type fromexecutionrather than a slug lookup is what lets a Portal report added to the server later be recognized without a cc-data release.Verification
13 files, +537/-30. 8 new tests; every one was verified to go red under a deliberate break.
go build,go vet,gofmt -landgo test ./...are clean on the head commit.Measured on the head commit: core 17,003 bytes, skill 20,291, MCP instructions 20,539, which is +2,669 / +3,385 / +2,782 against
main. The skill grows most because it alone carries the command spellings the core is forbidden to name. The full recipe was drafted both ways before being written: the minimal decision rule was +765 bytes against the full recipe's +1,532, which is not where a context budget is won or lost.The spec is closed at
specs/REPORT-95-portal-reports-guidance.md.