docs(dev): fix stale structure.md and document drapi client conventions - #879
Draft
ajalon1 wants to merge 9 commits into
Draft
docs(dev): fix stale structure.md and document drapi client conventions#879ajalon1 wants to merge 9 commits into
ajalon1 wants to merge 9 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.
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.