You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
REPORT-146: cc-data package commands and the run_answers view - #18
Researchers can now write a Researcher Dashboard package on a laptop, run it the way the dashboard's runner will, and publish it to the package catalog with the cc-data token they already have. Before this, a package was built and uploaded by a script that called AWS directly, carried a copied SQL library (_lib), and a mistake only showed up on a VM.
cc-data package init [dir] writes a manifest.json skeleton and a stdlib-only run.py stub that is a complete package: it reuses or creates a Student ID Mapping run for the scope's class, re-reads it live, pulls its answers, and counts through cc-data query.
cc-data package run [dir] --dataset <ref> --scope <file> runs the package under the runner's rules: a staged copy of exactly what build ships, the runner's scope.json and environment variables, the time bound, and the 65,536-byte cap on display.md. The package runs in its own process group, which is killed at the time bound, on Ctrl-C, and once the entrypoint exits.
cc-data package build [dir] zips the package reproducibly (same tree, same checksum), keeps each file's execute bit, never ships dot paths, __pycache__, *.pyc or local-data/, and refuses links.
cc-data package publish <zip> posts the zip to POST /api/v1/packages with the cc-data token, never retries, and checks the checksum report-server records.
The run_answers view replaces _lib's run-scoped answers query: every answer with the run_id of each run whose answers fetch holds it, always bound so a run with no answers counts zero. The guidance documents the learner count through the existing student_id_mapping view (by user_id, never by endpoint) and the log freshness query.
release.yml holds pre-release tags back. A tag with a - after the version (the planned v0.3.0-pre.1, which the runner image and REPORT-147 pin) still uploads every archive, but its GitHub release is marked as a pre-release and the Homebrew formula step is skipped.
The rules stay in report-server
cc-data keeps no copy of the catalog's package rules and no URL matcher. build and run send the zip to POST /api/v1/packages/validate, and run asks POST /api/v1/packages/applies whether the manifest's patterns apply to the scope. Both routes belong to REPORT-167 and are not deployed yet. A report-server without them answers 404, which both commands report as a warning and continue past, so this PR works today and gains its checks when REPORT-167 lands. An already-published version or a portal that cannot store packages yet is a warning from build and is ignored by run, since running needs neither.
What run cannot reproduce
The package runs as the researcher, with no network sandbox, and with a short passthrough list (HOME, PATH, the locale, the keyring's and the proxy variables) so its own cc-data calls find the stored login. A private .cc-data-run/bin/, holding only a link named cc-data to the running binary, goes first on the package's PATH, so the package calls the same cc-data rather than an older install (under go run too) and nothing else moves ahead of the user's PATH. A .py entrypoint runs with python3.11 when it is on PATH, as on the VM, else python3 with a note. A clue_prepull package is refused locally. package run --help and the new researcher-guide section both say this.
Worth a look
.cc-data-run/ and .cc-data-build/ each get a .gitignore of *, because a package directory is usually a git checkout and display.md is drawn from student data. Every directory run empties, and both .gitignore files, must be real rather than links, so a stray link cannot aim a removal or a write outside the package. .cc-data-run is also private, like every other folder cc-data keeps student data in: it and every folder run creates in it are 0700 (tightened if an earlier run left them wider), and scope.json and .gitignore are 0600.
Errors from report-server keep their code and exit class (5, or 3 for NOT_AUTHENTICATED). A package that does not apply, fails, or writes a refused result exits 1 with PACKAGE_REFUSED, PACKAGE_FAILED or PACKAGE_OUTPUT_REFUSED, so no new exit code joins the CLI's contract.
The researcher guide's sections after the new one renumber from 8.
Testing
go build ./..., go vet ./... and go test ./... pass, and go vet is clean for internal/packages under GOOS=windows and GOOS=darwin. Each test the plan names was checked by breaking the line it guards and watching it fail. The init stub was also run end to end against a fake cc-data, and package init through the built binary.
Not covered here, because it needs the release or REPORT-167: tagging v0.3.0-pre.1 after merge, a publish to staging, and the validate and applies refusals against a live server. VM parity moved to REPORT-147.
Requirements and implementation spec for cc-data's package commands and
the two dataset views that replace the spike's _lib queries.
The views are run_answers (every answer with each run whose membership
holds it) and learner_endpoints (one row per learner per assignment of
an answers report, counted by user_id). They reproduce _lib's answer and
learner counts on staging data, register empty on any dataset, and
materialize. Log freshness needs no view: logs.event_time already exists.
package init writes a manifest and a stdlib-only run.py that creates or
reuses its Student Answers run and counts through the new views. package
run stages exactly what build ships and gives it the runner's scope.json,
environment, time bound, display.md cap and result reading. build zips
reproducibly and keeps each file's execute bit, which unzip restores on
the VM. publish posts the zip to report-server with the cc-data token and
checks the recorded checksum.
The package rules and applicability are report-server's alone: build and
run ask a validate route and an applies route that a companion
report-service story adds, with one matcher and the existing profile
deriver behind them. Until those routes exist, a 404 is a warning. VM
parity moves to REPORT-147.
The plan was built and tested in a scratch copy of main before it was
written down; the mutations each step's tests catch are listed with it.
The companion report-service story now exists as REPORT-167, so the spec
names it where it said the story was still to be created. Dependencies
gain the one contract fixture REPORT-167 holds, and init's name check is
recorded as the one copy of that grammar cc-data keeps on purpose.
REPORT-167's spec refined the validate route. A version that is already
published is reported with already_published, and a portal that cannot
store packages yet is reported with publishing_unavailable; neither is a
refusal, so package run keeps working on a published version and on a
portal without a bucket, staging included. Validate takes official as
publish does. build now validates with --origin and --official as the
publish will, warns on either condition and still writes the zip; run
ignores both. The applies body always carries urls, which REPORT-167
requires, and neither route answers 404 on a server that has it, so a
404 still means only that the route is not deployed.
REPORT-147 measured on production that a whole-class Student Answers run
fails above three or four assignments and that a reused run's learner
list goes stale, and counts learners through a Student ID Mapping run.
The learner_endpoints view is therefore dropped: nothing would use it,
and the existing student_id_mapping view joined to run_answers already
counts learners by user_id. The guidance documents that count, and the
init stub now reuses or creates a Student ID Mapping run for its class,
re-reads it live and counts through run_answers and student_id_mapping.
A second review round, checked against the plan's code in a scratch
build, fixed six things. package run keeps an applies error's server
code and exit class instead of reporting INTERNAL. Ctrl-C now cancels
the run and kills the package's process group, which the terminal's
own interrupt cannot reach. build refuses a manifest name or version
that would turn its default output into a path. The proxy variables
join the passthrough list, as the runner sets them. R18 and the zip
comment admit the fixed extended-timestamp field, and publish drops its
local copy of report-server's origin rule.
The runner and REPORT-147 now pin a pre-release tag, v0.3.0-pre.1, cut
after this story merges, rather than waiting for 0.3.0 (Doug, stream
channel #21 and #22). R25 and a seventh plan step make that tag safe:
release.yml classifies the tag once, marks a pre-release as one on
GitHub with every archive and checksum attached, and skips the Homebrew
formula for it, with a test pinning both uses.
Following RD-4 pass 2's re-spec (stream channel #28 and #29): once the
entrypoint exits, package run kills what is left of its process group
before reading the output, as the runner kills every process of the
package's uid. The passthrough's NO_PROXY is credited to the laptop,
since the runner sets only the HTTP(S)_PROXY spellings, scope.json's
bare dataset name is recorded as settled, and Done when no longer waits
on staging's PackageBuckets, which is now set.
…-146]
run_answers is every answers row with the run_id of each run whose
answers fetch holds it in membership. It is registered on every dataset,
so a run with no answers counts zero rather than failing to bind as
answers_<run> does, and it declares no files, so a materialized dataset
reads it through the materialized answers and run_membership.
The guidance and the researcher guide document it with the learner
count through student_id_mapping, counted by user_id, and the per-run
log freshness query. The tests run both counts on a dataset where each
wrong way of counting gives a different number.
internal/packages gains the local side of a Researcher Dashboard
package, with no callers yet:
- ReadManifest reads only the fields a run acts on: the entrypoint,
which must be a collected file, and a positive integer duration.
Every other rule stays report-server's.
- Collect gathers what build ships and run stages, leaving out dot
paths, __pycache__, *.pyc and local-data, refusing links and other
non-regular files, and recording each file's ship mode (0755 when
any execute bit is set, else 0644).
- Zip writes a reproducible archive, and CheckEntrypointMode refuses a
non-.py entrypoint that would not be executable after unzip.
- ReadResult reads display.md, summary.txt and counts.json as the
runner does, following no link and refusing a display.md over the
65,536-byte cap.
The spec's manifest check drops its raw-bytes duration test, which
json.Unmarshal already makes redundant.
packages.Run is package run's engine:
- It refuses a clue_prepull package, and asks an injected Applies for
a package that declares patterns: a "does not apply" verdict is
refused with the runner's prefix, and an unconfirmed one warns and
runs.
- It rebuilds .cc-data-run (a staged copy of what build ships, in/,
out/, a persistent data/ and a .gitignore of *), refusing any linked
directory before removing anything, and writes the runner's
scope.json from the author's four-key scope file.
- It runs the entrypoint from the staged copy with the runner's
variables plus a short passthrough list (credential lookup, locale,
proxies), with python3.11 or python3 for a .py entrypoint.
- The entrypoint runs in its own process group, which is killed at the
time bound, on an interrupt, and once the entrypoint exits.
Client.do delegates to a new send that names the body's media type, so
the zip routes share the bearer, the cross-origin redirect refusal, the
no-retry rule for POST and the error-envelope decoding with every other
call.
PublishPackage and ValidatePackage post the raw zip as
application/zip, with origin and official as query parameters only
when given. PackageApplies posts the patterns and the scope's
assignment URLs. RouteMissing recognizes the 404 a report-server
without the validate and applies routes answers.
The command tests exercise the requests.
- init writes a manifest.json skeleton and an embedded, stdlib-only
run.py stub. The stub reuses or creates a Student ID Mapping run
filtered to the scope's class, re-reads it live, pulls its answers
and counts through run_answers and student_id_mapping.
- run checks the package with report-server's validate route, asks its
applies route through a 5-minute call for the deriver, and runs the
package under the runner's layout. An interrupt cancels the run so
the package's process group dies with it, and errors keep the
server's code and exit class.
- build zips reproducibly, validates with the --origin and --official
the publish will use, warns on an already published version or a
portal that cannot store packages yet, and writes to
.cc-data-build/<name>-<version>.zip unless --out says otherwise. A
name or version that would make that a path is refused.
- publish posts the zip with the cc-data token, never retries, and
checks the checksum report-server records.
A report-server without the validate or applies route answers 404,
which build and run report as a warning and continue past.
The researcher guide gains "Developing and publishing a package": the
init, run, build and publish loop, the four-key scope file and where to
keep it, what package run reproduces of the runner and what it cannot,
the checks report-server makes and the warning an older one gives, the
"publishing is not configured" answer, and the student data the two
working directories can hold. Later sections renumber from 8.
The README's command sketch and the Claude skill gain the four
commands, and package run's help now states the local/VM differences
that only package's help stated.
The runner image and REPORT-147's publish workflow pin v0.3.0-pre.1,
cut after this branch merges. release.yml now classifies the tag once:
a tag with a "-" after the version still builds and uploads every
archive and checksum, but its GitHub release is marked as a
pre-release, so it is never shown as latest, and the Homebrew formula
step is skipped, so brew upgrade never offers it. The archives upload
inside gh release create, before the formula step, so skipping the
formula cannot drop them.
A release_test.go guard pins both uses of the classification, and the
README's release paragraph says what a pre-release tag publishes.
- run_answers.run_id is cast to BIGINT, as the fallback view already
was, so its type no longer depends on whether a dataset has
membership.
- An unreadable scope file is INVALID_SCOPE, like an invalid one.
- A .gitignore that is a link is refused rather than written through,
and build refuses a .cc-data-build that is a link, so neither working
directory can be used to write outside the package.
- publish's usage line and the README sketch name --json.
New tests cover the BIGINT column, both link refusals, the python3
fallback note, the build .gitignore, the applies warnings for unread
and truncated profiles, publish's exit 3 and its retry advice when the
server never answered, and the unreadable scope file. The requirements
now say that a linked summary.txt refuses the result and a linked
counts.json is ignored, as on the runner, and reconcile the deriver's
240 seconds with report-server's 270.
package run's body after the credential lookup moves into
packageRunFlags.run(ctx, client, ...), as build and publish already
have, with the package and scope file read and checked by load before
any credential is needed. A new test runs a shell package against a
fake report-server and checks the validate call, the names and server
the package sees, and the stdout result line; skipping validate or
swapping the ref and its bare name now fails it.
environment uses slices.Contains in place of a hand-rolled helper.
The requirements and implementation plan are folded into one closed
spec, specs/REPORT-146-package-commands-and-dataset-views.md, holding
the requirements, Done when, technical notes, out of scope, what waits
on the merge or on REPORT-167, and every decision with its rationale.
The source folder is removed.
- The documented learner count no longer filters student_id_mapping by
run_id. That view keeps each learner's row from whichever mapping run
was fetched last, so the filter dropped a learner whom a later
mapping run of the same class also held. The answers are already
scoped by run_answers.run_id, and a run's answers belong only to its
own learners. The init stub's empty-scope guard counts the run's own
report_<run> for the same reason. The test gains a later mapping run
of the same class, so filtering the mapping by run now fails it.
- build writes the zip to a temporary file and renames it into place,
so a link already at the output path is replaced, not followed.
- A scope file must hold one JSON object and nothing after it.
- result.go says why summary.txt and counts.json are read under the
display cap: the runner reads them that way too.
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍 There are some issues to fix though. See Claude-generated review below for details. I vacillated a little on blocking because I know the risk related to the local file permissions is relatively low. I think fixing that should be straightforward and follow already-established patterns, though, and we should of course do all we can to secure sensitive data.
The four commands are well structured. The work to refuse symbolic links ("links") is careful, and the plan to keep the package rules in report-server is clear. Most tests check what their names say.
One blocker: the run folder holds student data, but it is not created with the private permissions that every other student-data folder in cc-data uses.
The other findings should be fixed, but they do not block the merge:
A relative data root points the package at the wrong folder.
--out accepts paths that fail or that the next run deletes.
Any 404 from report-server is treated as "route not deployed".
The tests do not cover the new error codes or a package that fails.
Blocker
1. The run folder, which holds student data, is not kept private
.cc-data-run and its subfolders are created with mode 0755, and scope.json is written with mode 0644. The run folder holds display.md, the class hash in scope.json, and the pulled data. The PR description says that display.md "is drawn from student data".
specs/REPORT-77-cc-data-cli.md:11 says the CLI classifies its local datasets as sensitive student data and ships with the file permissions that this classification requires. The code follows this everywhere else. fsutil.EnsureDir (internal/fsutil/fsutil.go:56-58) creates folders with 0700 "(its contents are sensitive)", and the dataset, store, attachment and materialized folders are all created with 0700. This PR would be the first place in cc-data that writes student data with more open permissions.
On a single-user laptop the real risk is low, because it depends on the parent folder and the umask. But the code does not make sure that the data is private, and we do not want to weaken the student-data protections.
Fix: Use fsutil.EnsureDir and mode 0600 for .cc-data-run and the files written in it. .cc-data-build holds the zip that is published, and local-data/ is never in it, so its mode is your choice.
Should fix
2. A relative CC_DATA_ROOT points the package at a different folder
The user sets CC_DATA_ROOT=./data, or sets a relative data_root in the config. DataRootDir returns the relative path unchanged.
cc-data finds the dataset under <cwd>/data.
The package runs from <pkg>/.cc-data-run/pkg. Its own cc-data calls use .cc-data-run/pkg/data.
package run deletes .cc-data-run/pkg before each run, so the data the package pulls is lost every time.
The default data root (~/cc-data) is absolute, so this occurs only with a relative setting. But it fails without an error, so even an experienced user gets no sign that something is wrong. All run tests use the absolute path "/data/root". The comment on RunOptions.DataRoot (run.go:64) says "as package run resolved it", which suggests an absolute path.
Fix: Pass filepath.Abs(dataRoot). Add a test that uses a relative root.
3. --out accepts the package folder and paths inside .cc-data-run
internal/packages/build.go:28
--out mypkg gives the relative path ".". excluded(".") is true because the name starts with a dot, so the check passes. The rename then fails with an internal error, not a usage error.
A path inside .cc-data-run/pkg/ also passes the check. The next package run deletes that file without a warning.
TestPackageBuildRefusesAnOutputItWouldCollect does not test either case.
4. Any 404 is treated as a missing route
internal/api/packages.go:104
RouteMissing returns true for every 404. A report-server without the validate and applies routes answers an unknown /api/v1 path from its catch-all route with 404 {"error":"NOT_FOUND","message":"Not found."}. A real route that calls ErrorHelpers.not_found sends the same body. So the client cannot tell the two cases apart. No body, header or error code is different.
No route causes this today: publish answers 403, not 404, for an origin it does not accept. But if validate or applies ever answers 404, cc-data reports "route not deployed yet" and builds or runs without the check. This affects validate (cmd/package.go:198) and appliesFor (cmd/package.go:218).
Fix: Agree with REPORT-167 that these two routes never answer 404. They should use 403 or 422, as publish does. Write that agreement in the comment on RouteMissing (internal/api/packages.go:102-103). As a narrower check, you can also match the exact body of the catch-all, but this only reduces the risk.
5. A package folder given as a link gives a misleading error
internal/packages/archive.go:63-65
filepath.WalkDir does not go into a starting folder that is a link, so Collect finds no files. os.ReadFile on manifest.json follows the link and works. The user then sees entrypoint "run.py" is not a file in the package, which is not the real problem. Resolve the link with filepath.EvalSymlinks first, or refuse it with a clear message.
6. The package can call a different cc-data than the one that runs it
The template calls cc-data by name, so it uses the first cc-data on PATH. A developer who runs a new build while an older Homebrew copy is on PATH gets a failure from the template's query on run_answers, because the older copy does not have that view. The error does not mention the version.
Adding the folder of os.Executable() to the start of the package's PATH fixes this for a built binary. It does not fix it for go run or a test binary, because those binaries are not named cc-data. If this case matters, consider an environment variable that gives the package the full path of the running binary.
7. init gives the wrong reason when it cannot check the folder
internal/packages/init.go:30-32
Every Lstat error other than "does not exist" is reported as "manifest.json already exists". If foo is a regular file, the real error is "not a directory", but the user sees the wrong message. Return the real error.
8. replaceFile has no Windows rename retry
internal/packages/build.go:64
This function uses a plain os.Rename. fsutil.RenameAtomic already retries the rename on Windows, where a virus scanner can hold the old zip open for a short time. Reuse the fsutil rename helper. The zip does not need the 0600 mode of fsutil.WriteFileAtomic0600. This is low priority.
Missing tests
These claims in the PR description have no test:
T1. The three new error codes.PACKAGE_REFUSED, PACKAGE_FAILED and PACKAGE_OUTPUT_REFUSED occur only in cmd/package.go:242-246 and the spec. No test checks the codes or the exit code 1. The PR says these are part of the CLI's contract.
T2. A package that fails. No test runs an entrypoint that exits with a non-zero status. No test covers a run that writes no display.md.
T3. "Never retries". The request count is checked after a 409, which is never retried. The dropped-connection case in TestPackagePublishMapsAuthAndUnansweredFailures, where a retry could happen, does not count requests. The client does not retry a POST today, so this is a gap in the tests, not a bug.
These are suggestions for stronger coverage, not separate defects:
T4.internal/duck/run_views_test.go:14 copies the documented SQL query. The same query is also in core.md and in template/run.py, so a change to one copy is not caught.
T5.release_test.go only searches release.yml for exact text. It does not test how a tag is classified (see N1).
T6.TestPackageRunKeepsTheServersCode calls packageRunError with errors built in the test. It does not send a server error through appliesFor or validate.
T7.TestRunLeavesNothingBehind depends on timing (a write after 1 s, a check after 1.5 s). On a slow CI machine the write can come after the check, so the test can pass when the code is broken.
T8. Smaller gaps:
TestRunGivesThePackageTheRunnersLayout passes HOME in but never checks it.
The clue_prepull case in TestRunRefusesBeforeStarting does not check that the entrypoint did not start.
TestPackageRunValidatesThenRunsWithTheRunnersNames does not check that validation happens before the run.
T9. No test checks the permissions of .cc-data-run and its files (see finding 1). Other tests in the codebase check 0600 on non-Windows systems.
These tests are good:
The link tests check that the files behind the link are not changed.
The time-limit and Ctrl-C tests check the process-group kill indirectly.
The test data in the run_answers test is built so that each wrong way to count, including filtering the mapping by run, changes an asserted count.
The fake server in cmd/package_test.go answers an unknown path with the same 404 body as report-server's catch-all route.
Comments and help text
C1. proc_windows.go:7 describes behavior that does not exist. It says the functions "kill only the entrypoint", but both are empty. exec.CommandContext kills the entrypoint. reapGroup kills nothing.
C2. result.go:53 is in the wrong place. The comment about summary.txt and counts.json is above res.DisplayBytes = size. Move it above the summary.txt read.
C3. run.go:136 says "nothing the package started outlives it". This is false on Windows. It is also false on Unix for a child that starts its own process group, for example with setsid.
C4. run.go:152 says prepare "empties and rebuilds the run tree".data is kept from one run to the next. Please say so.
C5. The short help for package run says too much.Short says "exactly as the dashboard's runner will". The long help lists the differences, but it does not say that a clue_prepull package is refused locally. The PR description says that it does.
C6. archive.go:61-62 says a link is "refused rather than skipped". The code checks the name against the excluded names first, so a link named .env or x.pyc is skipped without an error.
C7. run.go:26 says "Matching is case-insensitive". The LC_ prefix match is case-sensitive.
C8. result.go:14-15 refers to final-design.md section 10. That file is not in this repository. Give the repository name or a link.
Nits
N1. release.yml:178.*-* matches a hyphen in build metadata. For example, v1.2.3+ci-build would be marked as a pre-release and would not go to Homebrew. No tag uses + today, and this mistake only holds a release back, never publishes one by mistake. If you fix it, check the part before +: [[ "${GITHUB_REF_NAME%%+*}" == *-* ]]. release_test.go locks in the exact *-* line, so it would also need to change.
N2. internal/guidance/src/core.md:140 is 128 characters long. The other lines in the file are 89 characters or fewer. Please wrap it.
N3. Files.Bytes (archive.go:30, archive.go:97) is calculated but never read. A reader could think a size limit is checked locally. Remove it.
N4. template/run.py:57 ignores any dataset create error whose text contains "exists". cc-data's own messages use "already exists", but an OS error such as "file exists" would also be ignored. Match "already exists".
Suggested order
Finding 1 (blocker), with a permissions test (T9).
Findings 2 and 3.
T1 to T3.
Finding 4: agree on the 404 rule with REPORT-167 before that work is merged.
Findings 5 to 8.
Comments, the other test suggestions, and nits, in any order.
- .cc-data-run and every folder package run creates in it are 0700,
tightened if an earlier run left them wider, and scope.json and
.gitignore are 0600, as every other student-data folder is.
- package run makes the data root absolute before the package sees it,
since the package runs from its staged copy.
- build's --out refuses a directory and any path inside .cc-data-run.
- A package folder that is itself a link is resolved before it is
walked; links inside it are still refused.
- When the running binary is named cc-data, its folder goes first on
the package's PATH, so the package calls the same cc-data; under any
other name, run warns which one the package will reach.
- init reports the real error when it cannot check the folder, and
build renames its zip through fsutil.RenameAtomic.
- RouteMissing's comment records that validate and applies never
answer 404 themselves.
- release.yml ignores build metadata after "+" when it classifies a
tag, and a test now runs that step against sample tags.
New tests cover the three package error codes, a package that fails
or writes no display.md, publish sending once when the connection
drops, an applies 503 through package run, validate running before the
run, HOME, the run folder's modes, and the learner query staying the
same in the test, the guidance and the stub. The leftover-process test
checks that the process is dead rather than racing a timed write.
Comments and help text that overstated behavior are corrected,
Files.Bytes is removed, and the stub ignores only "already exists".
Thanks Ethan, all of it is addressed in c6f1d6a. Each fix has a test that fails when the fix is reverted.
Blocker
.cc-data-run and every folder package run creates in it are now 0700, and a folder an earlier run left at 0755 is tightened. scope.json and .gitignore are written 0600 through fsutil.WriteFileAtomic0600. I kept ensureRealDir rather than switching to fsutil.EnsureDir, because these folders must refuse a link and MkdirAll follows one, but the mode is the same 0700. TestRunKeepsItsTreePrivate checks every folder and file, starting from a pre-existing 0755 folder (T9).
Should fix
package run makes the data root absolute before the package sees it, and the end-to-end run test now passes a relative root.
--out refuses a directory (the package folder included) and any path inside .cc-data-run, both as usage errors.
REPORT-167's spec already makes this agreement: neither new route ever answers 404, and both refuse with 403 or 422 as publish does. RouteMissing's comment now states that. I didn't add the body match, since, as you say, a real not_found sends the same body.
Collect resolves a package folder that is itself a link. Links inside the package are still refused.
When the running binary is named cc-data, its folder goes first on the package's PATH. Under any other name (go run, a test binary), run warns which cc-data the package will reach, or that none is on PATH.
init returns the real Lstat error, so a regular file reports "not a directory".
The zip's rename goes through fsutil.RenameAtomic.
Tests
T1: TestPackageRunErrorsAreTheRunnersCodesAtExitOne checks all three codes at exit 1.
T2: TestRunReportsAFailedPackageAndAMissingDisplay covers a non-zero exit and a run that writes no display.md.
T3: the dropped-connection publish now counts its requests (one).
T4: TestTheDocumentedLearnerCountIsTheTestedOne checks that the query in core.md and the one in template/run.py match the tested one, anchored on each copy's closing delimiter so extra SQL fails it too.
T5 and N1: the classification step now checks the part before +, and TestReleaseWorkflowClassifiesTags runs it with bash against v0.3.0-pre.1, v0.3.0 and v1.2.3+ci-build.
T6: TestPackageRunPassesAnAppliesFailureThrough sends a 503 from applies through run and gets SERVICE_UNAVAILABLE at exit 5. A validate refusal through build was already covered.
T7: the leftover-process test now records the background process's PID and waits up to 5 seconds for it to die, counting a zombie as dead. Removing the kill fails it.
T8: HOME is checked, the clue_prepull case checks that the entrypoint never started, and the run test checks that .cc-data-run does not exist yet when validate answers.
Comments and help text
C1 to C8 are fixed as you suggested. package run's short help no longer says "exactly", and the local/VM differences now mention the clue_prepull refusal. N2 to N4: core.md is wrapped, Files.Bytes is gone, and the stub matches only "already exists".
Also tested locally, through a built binary named cc-data
package build against production's report-server: validate answers 404, build warns and writes the zip, the unzipped run.py stays 0755, a rebuild after touch gives the same checksum, and all three --out refusals are usage errors.
Ctrl-C to the terminal's process group during package run: cc-data and the package are both gone, and stdout carries PACKAGE_FAILED "the package was interrupted".
The init stub against production's test class (run 2399), with a relative CC_DATA_ROOT and an older cc-data first on my PATH that has no run_answers: it reported 87 answers from 3 learners, the data landed under the absolute root, and the run folder was 0700 with 0600 files.
TestPackageRunReachesThisCCData compared filepath.Dir's answer with a
literal "/build", which Windows spells "\build". The expected folder is
now joined with the platform's separator.
This positivity check does not ensure the duration can be represented by the later time.Duration(seconds)*time.Second + TimeoutMargin calculation. While the validation route is unavailable, a manifest value above 9,223,371,436 is accepted and overflows that calculation, causing an immediate or otherwise incorrect timeout instead of the declared bound. Reject values that cannot represent the complete local bound before running.
On a case-insensitive file system, the default on macOS and Windows,
--out .CC-DATA-RUN/x.zip lands inside .cc-data-run, where the next
package run deletes it, but passed the case-sensitive comparison. The
first path component is now compared without case on every platform,
and the --out test covers the upper-case spelling.
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 Claude had the below to say about the latest state of things, but it's all minor stuff that I'll leave to your discretion.
Adding the binary's folder to PATH has a side effect. If cc-data runs from a shared folder such as /opt/homebrew/bin, every tool in that folder comes first on the package's PATH. That includes python3, if the package calls it itself. The interpreter for the entrypoint is not affected, because the parent process chooses it. A private folder that holds only a link to the binary would avoid this. This is low risk.
The --out check does not resolve links. If the package folder and --out are given by different routes (one through a link, one by the real path), a path inside .cc-data-run passes the check. This is unlikely.
A blank line is left in a list in release_test.go. It is in TestReleaseWorkflowKeepsPreReleasesBack, where the old *-* string was removed. This is cosmetic only.
ranBeforeValidate in TestPackageRunValidatesThenRunsWithTheRunnersNames is shared between goroutines without a lock. The server's handler writes it, and the test reads it. CI runs go test ./... without -race, so this does not fail today. It would be reported if someone adds -race later. An atomic.Bool fixes it.
- package run no longer puts the running binary's own folder first on
the package's PATH, which moved every other tool in a shared bin
ahead of the user's PATH too. It links .cc-data-run/bin/cc-data to
the running binary, whatever its name, and puts only that folder
first. go run therefore needs no warning; Windows, which needs a
privilege to create links, keeps the binary's folder when it is
named cc-data and warns otherwise.
- build's --out check resolves links in the package folder and in
--out before comparing them, so the same place reached two ways
compares equal.
- A blank line left in a release_test.go list is removed, and the flag
the run test shares with its fake server's handler is an atomic.Bool.
The PATH side effect.package run now links .cc-data-run/bin/cc-data to the running binary and puts only that private folder first on the package's PATH, so nothing else from a shared bin moves ahead. As a bonus, the link carries the name, so a go run binary is reached too and its warning is gone. On Windows, which needs a privilege to create links, it keeps the binary's own folder when that is named cc-data, and warns otherwise.
--out through links.CheckBuildOutput resolves links in the existing part of both the package folder and --out before it compares them. TestCheckBuildOutputSeesThroughLinks covers both directions, plus an --out inside the package, and the old filepath.Abs comparison fails all three cases.
The blank line in TestReleaseWorkflowKeepsPreReleasesBack is gone.
ranBeforeValidate is an atomic.Bool. Strictly, the handler writes it only when the test is failing, which is why -race was quiet, but it was still wrong to share it without synchronization.
Retested locally before pushing:
Automated checks: the full suite, go vet for Linux, macOS and Windows, and -race on cmd, internal/packages and internal/api.
End to end with a rebuilt binary, with a fake python3 placed next to it:
The package's python3 still resolved to /usr/bin/python3, and its cc-data resolved to the link, both for a binary named cc-data and for one renamed main.
The --out checks refused a linked package folder paired with a real-path --out, and the reverse.
Ctrl-C, the build checksum, the init stub against the test class, and the 0700/0600 modes all behave as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
emcelroy
left a comment
Researchers can now write a Researcher Dashboard package on a laptop, run it the way the dashboard's runner will, and publish it to the package catalog with the cc-data token they already have. Before this, a package was built and uploaded by a script that called AWS directly, carried a copied SQL library (
_lib), and a mistake only showed up on a VM.Jira: https://concord-consortium.atlassian.net/browse/REPORT-146. Closed spec:
specs/REPORT-146-package-commands-and-dataset-views.md.What this adds
cc-data package init [dir]writes amanifest.jsonskeleton and a stdlib-onlyrun.pystub that is a complete package: it reuses or creates a Student ID Mapping run for the scope's class, re-reads it live, pulls its answers, and counts throughcc-data query.cc-data package run [dir] --dataset <ref> --scope <file>runs the package under the runner's rules: a staged copy of exactly whatbuildships, the runner'sscope.jsonand environment variables, the time bound, and the 65,536-byte cap ondisplay.md. The package runs in its own process group, which is killed at the time bound, on Ctrl-C, and once the entrypoint exits.cc-data package build [dir]zips the package reproducibly (same tree, same checksum), keeps each file's execute bit, never ships dot paths,__pycache__,*.pycorlocal-data/, and refuses links.cc-data package publish <zip>posts the zip toPOST /api/v1/packageswith the cc-data token, never retries, and checks the checksum report-server records.run_answersview replaces_lib's run-scoped answers query: every answer with therun_idof each run whose answers fetch holds it, always bound so a run with no answers counts zero. The guidance documents the learner count through the existingstudent_id_mappingview (byuser_id, never by endpoint) and the log freshness query.release.ymlholds pre-release tags back. A tag with a-after the version (the plannedv0.3.0-pre.1, which the runner image and REPORT-147 pin) still uploads every archive, but its GitHub release is marked as a pre-release and the Homebrew formula step is skipped.The rules stay in report-server
cc-data keeps no copy of the catalog's package rules and no URL matcher.
buildandrunsend the zip toPOST /api/v1/packages/validate, andrunasksPOST /api/v1/packages/applieswhether the manifest's patterns apply to the scope. Both routes belong to REPORT-167 and are not deployed yet. A report-server without them answers 404, which both commands report as a warning and continue past, so this PR works today and gains its checks when REPORT-167 lands. An already-published version or a portal that cannot store packages yet is a warning frombuildand is ignored byrun, since running needs neither.What
runcannot reproduceThe package runs as the researcher, with no network sandbox, and with a short passthrough list (
HOME,PATH, the locale, the keyring's and the proxy variables) so its owncc-datacalls find the stored login. A private.cc-data-run/bin/, holding only a link namedcc-datato the running binary, goes first on the package'sPATH, so the package calls the samecc-datarather than an older install (undergo runtoo) and nothing else moves ahead of the user'sPATH. A.pyentrypoint runs withpython3.11when it is onPATH, as on the VM, elsepython3with a note. Aclue_prepullpackage is refused locally.package run --helpand the new researcher-guide section both say this.Worth a look
.cc-data-run/and.cc-data-build/each get a.gitignoreof*, because a package directory is usually a git checkout anddisplay.mdis drawn from student data. Every directoryrunempties, and both.gitignorefiles, must be real rather than links, so a stray link cannot aim a removal or a write outside the package..cc-data-runis also private, like every other folder cc-data keeps student data in: it and every folderruncreates in it are 0700 (tightened if an earlier run left them wider), andscope.jsonand.gitignoreare 0600.NOT_AUTHENTICATED). A package that does not apply, fails, or writes a refused result exits 1 withPACKAGE_REFUSED,PACKAGE_FAILEDorPACKAGE_OUTPUT_REFUSED, so no new exit code joins the CLI's contract.Testing
go build ./...,go vet ./...andgo test ./...pass, andgo vetis clean forinternal/packagesunderGOOS=windowsandGOOS=darwin. Each test the plan names was checked by breaking the line it guards and watching it fail. Theinitstub was also run end to end against a fakecc-data, andpackage initthrough the built binary.Not covered here, because it needs the release or REPORT-167: tagging
v0.3.0-pre.1after merge, a publish to staging, and the validate and applies refusals against a live server. VM parity moved to REPORT-147.