Skip to content

Document the installId ArgumentException; test Json's name and tag-value limits - #7

Merged
dmccoystephenson merged 1 commit into
mainfrom
feature/constructor-throws-doc-and-json-length-tests
Oct 7, 2026
Merged

dmccoystephenson merged 1 commit into
mainfrom
feature/constructor-throws-doc-and-json-length-tests

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

Stage A (documentation accuracy sweep) plus a small Stage B (unit-test expansion) on the same surface: what the client rejects for length.

  • Doc drift fixed. The TraceClient constructor's XML <summary> stated that ArgumentException is thrown only for a missing/malformed baseUrl, a missing application, or a missing/overlong version. An installId longer than MaxLength also throws (TraceClient.cs, the explicitInstallId.Length > MaxLength check). The summary now lists it. The MaxLength constant's summary is extended to say the version and installation ID are held to it as well. Comment-only; no constant value, signature or behaviour changes.
  • Tests added (characterization, no production change):
    • Json_RejectsANameLongerThanTheLimitButNotOneAtIt: pins the name longer than 255 characters reason and the at-the-limit acceptance, which until now were only counted indirectly as a dropped log line.
    • Json_RejectsATagValueLongerThanTheLimitAndNamesItsKey: pins that an overlong tag value is rejected with a reason naming the key (tag level longer than 255 characters), and that a value exactly at the limit is written.

No tracking issue. The gap was found during triage (no open issues existed).

The rest of the docs sweep found no other drift. README tables ("What Report promises", "Turning it off", "The wire format", "Building"), the version strings (csproj, header, Version const, README User-Agent, all 0.3.0), and the build.yml / test csproj net48 comments were each checked against source.

Verification

  • dotnet is not installed in the dispatch sandbox, so the local anchor is UNVERIFIED. The Build matrix on this PR head (ubuntu net8.0, windows net8.0 + net48) is the gate.
  • No bug fix is included, so the stash-and-run regression check does not apply.

Test plan

  • Build / ubuntu-latest (net8.0) green, with the two new tests counted
  • Build / windows-latest (net8.0 + net48) green
  • GenerateDocumentationFile + TreatWarningsAsErrors still build cleanly with the edited XML docs

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

🤖 Generated with Claude Code


drafted by Claude on behalf of Daniel Stephenson

…lue limits

The constructor summary said it throws only for baseUrl, application and
version problems, but an installId longer than MaxLength throws too. The
MaxLength summary now says the version and installation ID are held to it.

Json's name-too-long and tag-value-too-long reasons had no direct test;
two characterization tests pin their wording and the at-the-limit case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric (scored against CI run 37432344507 on the PR head and the diff):

  • Scope: PASS. Two files changed: XML doc comments in src/TraceClient/TraceClient.cs (the constructor <summary> and the MaxLength <summary>) and two new tests in tests/TraceClient.Tests/TraceClientTest.cs. No formatting churn and no code changes.
  • Tests-new: PASS (not applicable to the code). No public member was added. The two new tests cover Json branches (name over the limit, and a tag value over the limit) that previously had no direct assertion.
  • Tests-fix: not applicable. No bug fix is included, and both tests are characterization tests that pass against the unchanged code.
  • Sibling structure: PASS. The new tests mirror Json_RejectsAnApplicationLongerThanTheLimitButNotOneAtIt and Json_RejectsATagKeyLongerThanTheLimitAndNamesOnlyItsStart in naming, shape and placement.
  • Sibling renames: not applicable. Nothing was renamed.
  • Docs: PASS. The README already says an overlong installId throws ("one over 255 characters throws ArgumentException, like an overlong version"), and the installId <param> doc already said so. Only the constructor <summary> "only" list disagreed, and this diff fixes it. No README change is needed.
  • Issue resolution: not applicable. There is no Closes #N, and the PR body states that the gap was found during triage.
  • CI: PASS. ubuntu net8.0 and windows net8.0 + net48 are all green, with 66 tests on each run (main had 64), which confirms both new tests executed on every target.
  • No dependencies: PASS. The csproj is untouched and no using was added.
  • Single file: PASS. src/TraceClient still holds only TraceClient.cs and TraceClient.csproj.
  • Vendoring floor: PASS. The csproj is untouched. With GenerateDocumentationFile + TreatWarningsAsErrors, the edited <paramref name="installId"/> and <see cref="MaxLength"/> resolve without warnings, which the green net48 leg confirms.
  • Never throws / non-blocking / environment seam / opt-out contract / version agreement: PASS. No executable line changed.

Judgment call for the reviewer: the new MaxLength summary says the constructor rejects a version or installation ID "past it". That holds for the explicit installId: argument. File-based IDs never reach the check, because InstallIdLine (^[A-Za-z0-9_.-]{1,255}$) already caps them at 255, so the wording stays accurate. A file line longer than 255 characters is treated as invalid and replaced, not rejected, and the doc does not claim otherwise.

Do-not-auto-merge check: the diff touches no wire-format, constant value, opt-out or signature lines, and no protected path matches. This dispatch is not authorized to merge, so the PR is left open for a human.

This review comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit cf9c27c into main Oct 7, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant