Repository navigation
Conversation
Add two task-oriented tutorials for researchers, from the first login to the results: one for the Krini portal and one for ga4gh-cli. Both run the same demo job, counting the words of a text file, as a TES task and as a WES workflow, and link to the Krini and CLI guides for all details. Add the demo CWL workflow and its parameters, tested with cwltool. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideConfigurationReview the MkDocs navigation entries for the tutorials alongside their placement in the researcher index.
DocumentationReview the two tutorials together with their shared CWL workflow and parameters: they walk through the same TES task and WES workflow in Krini and
Review how the researcher index introduces and links the new tutorials.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Drop the planned device-flow login from the CLI tutorial: the CLI reference documents only the token variable. Add warnings for the Krini gaps krini#66, krini#67 and cloud-components#574, and list both tutorials on the researchers start page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Documentation build overview
3 files changed+ researchers/tutorial-cli/index.html+ researchers/tutorial-krini/index.html± researchers/index.html |
uniqueg
marked this pull request as ready for review
November 13, 2026 22:06
Contributor
Linked issuesChecked at commit 28102f5 #111 docs: add end-user tutorials for the portal and the CLI · 1 of 2 met · 1 to check by hand
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/researchers/tutorial-cli.md" line_range="109-111" />
<code_context>
+with their `URL`s. If a service isn't listed, use the address that its
+operators gave you.
+
+Before you send a token to a service, check in the [front end of the Cloud
+Registry][elixir-cloud-registry] that it is verified: the CLI doesn't show
+this.
+
+Then add a `TES` and a `WES` section to `~/.ga4gh-cli/config.json` in a text
</code_context>
<issue_to_address>
**Login tokens reach untrusted services**
When a user relies on a verified Registry label for a service they have not independently trusted, including an unsynced or out-of-sync entry, the CLI sends `LS_LOGIN_TOKEN` to the configured service, so an attacker-controlled endpoint can reuse the bearer token to act as the researcher.
Tell users to independently verify the service endpoint and not treat the Registry's verified label as sufficient assurance before sending a token.
</issue_to_address>
### Comment 2
<location path="docs/researchers/tutorial-krini.md" line_range="154" />
<code_context>
+
+ | Field | Value |
+ |---|---|
+ | **Workflow URL** | `https://raw.githubusercontent.com/elixir-cloud-aai/docs/main/docs/researchers/tutorial-files/count-words.cwl` |
+ | **Workflow Type** | `CWL` |
+ | **Workflow Type Version** | `v1.0` |
</code_context>
<issue_to_address>
**Changed workflow executes remotely**
When an attacker alters or compromises the documentation repository's `main` branch before a user follows either tutorial, krini and `ga4gh-cli` submit the mutable `main`-branch CWL URL to WES, which executes its current contents. A malicious replacement therefore runs attacker-controlled workflow code on the selected service instead of the reviewed demo.
Pin the workflow URLs to an immutable revision and update them when the demo changes.
Also at `docs/researchers/tutorial-cli.md:254`.
</issue_to_address>
### Comment 3
<location path="docs/researchers/tutorial-files/count-words.cwl" line_range="10" />
<code_context>
+ DockerRequirement:
+ dockerPull: alpine:3.20
+baseCommand: wc
+stdin: $(inputs.text.path)
+inputs:
+ text:
</code_context>
<issue_to_address>
**Workflow fails on conforming engines**
When a WES service runs a CWL engine that enforces declared requirements, the CWL document uses the JavaScript expression `$(inputs.text.path)` for `stdin` but declares only `DockerRequirement`; an engine that honors CWL requirement declarations refuses to evaluate it, so the workflow fails before `wc` runs and both tutorials cannot produce their advertised result.
Add `InlineJavascriptRequirement: {}` to the document's `requirements`.
</issue_to_address>
### Comment 4
<location path="docs/researchers/tutorial-krini.md" line_range="143-144" />
<code_context>
+3. Point at the info icon next to the service and check that it supports
+ the workflow type `CWL` in the version `v1.0`.
+
+If no service that you can use runs CWL, ask its operator for an example
+workflow in a language that it runs, and use that in the next step.
+
+### 7. Start the workflow
</code_context>
<issue_to_address>
**Alternate workflows cannot be substituted**
When the available WES service runs another workflow language but not CWL v1.0, the next step still requires the supplied CWL workflow and `CWL` v1.0 settings, so the operator-provided workflow in another language is never used and the run is rejected.
Either make a CWL v1.0 service a strict prerequisite and remove the alternative-language advice, or explain how to substitute the operator’s workflow, language, version, and parameters in each tutorial.
Also at `docs/researchers/tutorial-cli.md:243-244`.
</issue_to_address>
### Comment 5
<location path="docs/researchers/tutorial-cli.md" line_range="196" />
<code_context>
+ "command": ["wc", "/data/LICENSE"]
+ }
+ ]
+}' > count-words-task.json
+TASK_ID=$(ga4gh-cli tes create count-words-task.json)
+echo "$TASK_ID"
</code_context>
<issue_to_address>
**Cleanup deletes users’ existing files**
When two tutorial sessions share a working directory, or either output filename already exists there, the fixed-name redirects and `curl -O` overwrite existing files, and cleanup deletes them; with shared sessions, one session can also remove `count-words.json` before the other reaches `wes run`, so the run cannot start.
Use a separate working directory or unique filenames for each session, and ensure cleanup only removes files created by that session.
Also at `docs/researchers/tutorial-cli.md:252`, `docs/researchers/tutorial-cli.md:296`.
</issue_to_address>
### Comment 6
<location path="docs/researchers/tutorial-cli.md" line_range="125" />
<code_context>
+ "base_path": "https://elixir-on.cloud/ga4gh/registry/v1"
+ },
+ "TES": {
+ "base_path": "https://<TES HOST>/ga4gh/tes/v1",
+ "oauth_access_token_env": "LS_LOGIN_TOKEN"
+ },
</code_context>
<issue_to_address>
**Service URLs become malformed**
When a user substitutes a registry `URL` that already includes `https://` and an API path into `<TES HOST>` or `<WES HOST>`, `base_path` then contains a duplicated scheme or API path because the template also adds those components; `ga4gh-cli` requests fail to reach the service, so the tutorial’s service checks and runs fail.
Tell readers to use the registry URL verbatim as `base_path`, adding an API path only when the URL stops before the API root.
Also at `docs/researchers/tutorial-cli.md:129`.
</issue_to_address>Sourcery assessment
Approval pending. 6 findings to address first.
Blocking findings: docs/researchers/tutorial-cli.md:111, docs/researchers/tutorial-krini.md:154, docs/researchers/tutorial-files/count-words.cwl:10, docs/researchers/tutorial-krini.md:144, docs/researchers/tutorial-cli.md:196, and 1 more
Warn in the CLI tutorial to send the token only to trusted services, explain base_path as the registry URL plus the API path where needed, keep the demo files in their own folder, and make CWL v1.0 a prerequisite of part 2 in both tutorials. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adds two end-user tutorials for researchers, from the first login to the results:
docs/researchers/tutorial-krini.md: Tutorial: Your First Run in Krini, in the browser.docs/researchers/tutorial-cli.md: Tutorial: Your First Run from the Command Line, withga4gh-cli.Both run the same small demo job (count the lines, words and characters of a public text file) twice: as a TES task and as a WES workflow. Each step does one thing, says what to check, and links to the reference guides for details instead of repeating them: the Krini user guide (#137,
docs/researchers/krini.md) and the CLI guide (#136,docs/researchers/cli.md), both merged.The demo materials are new files in
docs/researchers/tutorial-files/: a CWLv1.0CommandLineTool (count-words.cwl,wcinalpine:3.20) and its parameters (count-words.json). I didn't reuse the classic hashsplitter demo (cwl-WEStests/workflows/workflow.hashsplitter, also in the retiredelixir-cloud-demos): it fails with the currentkubler/opensslimage, whose OpenSSL 3.5 no longer knows the digestsha(dgst: Unknown option or message digest: sha).Aligned with the merged guides:
stagingatc81a259). Warnings where the known gaps hit a step: the TEST SERVER entry (krini#67), no result files from the task form (fix(tes): send outputs from the create-run form cloud-components#574), and no run details in Krini, so the WES result is read through the API as the guide describes (krini#66). TES tasks need a service that accepts requests without a token.oauth_access_token_env); no device-flow login. Binaries are marked as planned (ci: publish CLI binaries on version tags ga4gh/ga4gh-sdk#91). The tutorial offers two ways to get a token: an own LS Login client, or copying it from Krini (if the service accepts it).docs/researchers/index.md: both tutorials listed in "What you can try", after "Use the Command Line".mkdocs.yml: two nav entries at the same place.A run-through of both tutorials by someone who didn't write them (the last criterion of #111) is welcome, e.g., at the BioHackathon, but doesn't block the merge: both pages say how they were tested and ask readers to report what didn't work.
Verification
On the branch after merging
origin/main(c0e5d36, with #135, #136, #137 and #138):python scripts/check_names.py: no findings.mkdocs build --strict: built without warnings (links and anchors tokrini.mdandcli.mdresolve).pytest: 25 passed, coverage 100%.cwltool 3.3with Docker rancount-words.cwlwithcount-words.json(input fetched from thehttpsURL) and producedcounts.txt=121 1066 7048, which matcheswc LICENSE. The TES task's command,wc /data/LICENSEinalpine:3.20, prints the output shown in the tutorials.af0e021). Until this PR is merged, the raw GitHub URLs ofcount-words.cwlandcount-words.jsononmainreturn 404.Sourcery review (6 threads): fixed 4 in 28102f5 (warning to send the token only to trusted services,
base_pathas the registry URL plus the API path where needed, demo files in their own folder, CWLv1.0as a prerequisite of part 2); the checks above were rerun after the fixes.Declined review findings
main; a SHA can't be pinned before the squash merge, and each later pin would need a follow-up PR. A tag can replacemainif the docs get releases.InlineJavascriptRequirementtocount-words.cwl(Sourcery):$(inputs.text.path)is a CWL parameter reference, which CWL v1.0 supports without JavaScript; cwltool ran the file as is.Light lane: docs only (two Markdown pages, two demo files, two index rows, two nav lines); no code, configuration or CI changes. I reviewed the diff myself.
Closes #111
🤖 Generated with Claude Code