docs(dev): fix stale structure.md and document drapi client conventions - #879
Open
ajalon1 wants to merge 10 commits into
Open
docs(dev): fix stale structure.md and document drapi client conventions#879ajalon1 wants to merge 10 commits into
ajalon1 wants to merge 10 commits into
Conversation
- structure.md: add workload/ and auth/ to the directory trees (both were missing despite being two of the largest packages in the repo), fix the auth/ worked example to match the current one-subdirectory-per-verb layout, and replace the Configuration code sample's nonexistent config.GetAPIKey/SetAPIKey/SaveConfig calls with the real viperx/ UpdateConfigFile pattern already documented in configuration.md. - Add docs/development/drapi-client.md: the dev-doc mirror of internal/drapi/doc.go covering the verb-function convention, timeout clamping, the HTTPError/errors.As contract (including the known Get() vs Post/Patch/Delete detail asymmetry), and the origin-safety check before attaching credentials to a server-supplied URL. - Cross-link the new doc from authentication.md, docs/development/README.md's Advanced topics, AGENTS.md's new Auth & API Client Conventions section, and a one-line worked-example pointer in .cursor/bugbot-security.md. A dedicated internal/workload architecture doc is being drafted separately for review by the Workload API team, so this change intentionally leaves that package's structure.md subsection as a placeholder pending that PR.
…erns - Terminal Errors Must Name the Exact Recovery Command - Retry Safety Must Be Classified by Response Shape, Not Status Code Alone - Heuristic Detection Must Degrade, Never Refuse - Duplicated Constants Need a Canonical-Source Comment - Document Intentional Asymmetry Between Look-Alike Siblings Each generalizes a pattern observed in real workload/auth/drapi code during the docs audit for datarobot-oss#879, kept domain-agnostic per the existing bugbot rule style.
Today's countflags migration (CFX-7834, datarobot-oss#858/datarobot-oss#859/datarobot-oss#860) is documented in docs/development/flags.md but had no reviewer-facing rule — a new count-like flag validated in RunE instead of at parse time would sail through review. Adds the parse-time-validation rule plus its documented exception (0 as a "use default" sentinel, per the workload config --port/--replicas precedent).
Broadens "count-like flags validate at parse time" to any flag shape that recurs across commands or shares a validation requirement wherever it appears — not just counts. countflags and pollflags are cited as the two existing precedents (counts, durations); the point is catching a third command re-implementing the same check by hand instead of reusing one of these or adding a new shared pflag.Value.
Codifies the pattern already present in the codebase: internal/drapi/filesapi and internal/workload/apiclient are thin typed wrappers built entirely on drapi's verb functions, while internal/auth/browserflow.go (local OAuth handshake) and internal/plugin/remote.go (plugin downloads) correctly build their own plain http.Client because they aren't DataRobot API calls. Adds both directions of the boundary so a reviewer catches a wrapper duplicating drapi's plumbing and an unrelated call wrongly forced through it.
ajalon1
commented
Aug 31, 2026
Comment on lines
+15
to
+17
| Worked example: `docs/development/drapi-client.md` — before attaching a bearer token to | ||
| a server-supplied URL (pagination cursor, live endpoint), verify it with | ||
| `drapi.URLMatchesConfiguredBase()` first. |
Contributor
Author
There was a problem hiding this comment.
Not sure I need or want worked examples in the bugbot cursor rules.
Prompted by CFX-7754 (datarobot-oss#820, merged today): the auth login callback listener on localhost:51164 accepted any request during its window, letting a hidden <img> on any open page plant a forged API key. Adds the provenance-validation rule for local OAuth-style listeners, plus a rule against security docs overclaiming what a mitigation blocks (the PR's own review round caught exactly that in its first docs draft). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ajalon1
marked this pull request as ready for review
September 11, 2026 00:28
Contributor
Author
|
/skip-smoke-tests |
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
1 participant
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.
RATIONALE
workloadandauthhave absorbed most of the last two months' commit volume, but the dev docs a new contributor actually browses hadn't kept up —structure.mdstill showed a directory tree and aconfig.GetAPIKey()/SetAPIKey()/SaveConfig()code sample that no longer exist (and contradicts the correct pattern already documented inconfiguration.md), andinternal/drapi's conventions (timeout clamping, theHTTPError/errors.Ascontract, origin-safety before attaching credentials to a server-supplied URL) lived only in a package doc comment, unlinked fromdocs/development/.This closes that gap with pure documentation — no code changes.
A dedicated
internal/workloadarchitecture doc is being drafted separately for the Workload API team to review and own, so this intentionally leaves that package'sstructure.mdsubsection as a placeholder pending that PR.CHANGES
docs/development/structure.md: addworkload/andauth/to the directory trees, fix theauth/worked example to match the current one-subdirectory-per-verb layout, replace the stale Configuration code sample with the realviperx/UpdateConfigFilepattern.docs/development/drapi-client.md: verb-function convention, timeout clamping, theHTTPError/errors.Ascontract (including a knownGet()vsPost/Patch/Deletedetail asymmetry, flagged not fixed), and the origin-safety check before attaching credentials to a server-supplied URL.authentication.md,docs/development/README.md's Advanced topics,AGENTS.md's new Auth & API Client Conventions section, and a one-line worked-example pointer in.cursor/bugbot-security.md.Note
Low Risk
Markdown-only changes to development guides and BugBot rules; no runtime, auth, or API behavior changes.
Overview
Documentation-only update to bring contributor docs and BugBot guidance in line with current
auth,workload, anddrapipatterns.Dev docs: Adds
docs/development/drapi-client.md(verb functions, timeout clamping,HTTPError/errors.As, origin checks viaURLMatchesConfiguredBase, and the knownGet()vs other verbs error-detail gap).structure.mdnow listscmd/workload,internal/auth, andinternal/workload, fixes the auth subcommand layout example, and replaces obsoleteGetAPIKey/SaveConfigsamples withviperx+UpdateConfigFile. Cross-links land inREADME.md,authentication.md, andAGENTS.md(new Auth & API Client Conventions section).BugBot / review rules: Expands several
.cursor/bugbot-*.mdfiles with shared-flag registration, sentinel defaults, intentional sibling asymmetry, terminal recovery commands, retry-by-response-shape,drapiextension vs re-implementation, non-platform HTTP clients, duplicated-constant comments, localhost callback provenance, scoped security claims, and heuristic validation that warns instead of hard-blocking.Reviewed by Cursor Bugbot for commit 2861434. Configure here.