Final review fixes: guard the logs resolution, narrow the swallow, settle the surface tiers - #11
Merged
Merged
Conversation
AGENTS.md and the 0.1.0 changelog entry both promise that a release moving
opentelemetry._logs costs the logs signal and nothing else: import guideme
still works and traces are untouched. The guard was narrower than that
promise. It covered the deferred import; SeverityNumber.INFO, SeverityNumber
.WARN and both get_logger calls sat outside it, at module scope through
LOGS = resolve_logs(). A module that still imports and has lost a member
therefore killed import guideme with an AttributeError, traces included.
Logs._emit had the matching hole. It wrapped the private LogRecord
constructor and logger.emit in one except Exception: pass, while its
docstring scoped the swallow to a sink that raises. A LogRecord that no
longer takes event_name then produced no record, no span event under
events("log"), and no error: the records that silently go nowhere the
refusal in GuideBuilder.events exists to prevent.
Every read of the private API now happens inside the one guard, which
catches ImportError, AttributeError and TypeError, and resolve_logs builds
one throwaway record with the five keywords _emit uses, so a constructor
change is found where absence is already representable. _emit builds its
record before the try, leaving the swallow over logger.emit alone.
Three gaps, all filled as parameters of tests that already existed, so the
43-function ceiling holds.
A LogRecord whose constructor moved is the drift ImportError cannot see, and
it is what used to make an ask emit nothing and say nothing. The new case
replaces LogRecord on the module, because resolve_logs reads the name off it
every call, and asserts the signal goes absent so events("log") refuses.
guide.py refuses an events(...) value outside span, log and both, and three
documents promise it, but nothing tested it. Events is a Literal, so the
case casts to reach the runtime guard, which is where a caller with no type
checker stands.
api/client.py rests on httpx dropping the authorization header when a
redirect changes origin, and only the same-origin case was covered. A second
HTTPServer on its own port makes the hop cross origins, and the case asserts
the key reaches none of the second server's headers.
ask's docstring listed "two runtime keys collide" under the ConfigError it raises. It does not raise it: validate(ChoiceSpec) refuses a repeated key while choose_among builds the question, long before anything is asked. A caller reading the generated overloads would look for the failure in the wrong place. The clause leaves ASK_DOC, which now says a rubric this call could not ask is refused where the question is built, and choose_among says the same from the other side.
The class docstring listed an events(...) value outside the three modes and
stopped there. events("log") and events("both") also raise it where the
installed opentelemetry-api provides no logs API, which is the case a reader
hitting it is least likely to guess. The list is open-ended and keeps that
shape.
Four documents disagreed with the code or with each other. design.md told callers to import Question from guideme.question while AGENTS.md and the README said the supported surface was __all__ plus guideme.api and guideme.policy, and everything else private. Question is what noul, choose, choose_among, score, score_levels and all three .detail() methods return, and a dict annotation cannot be written without it, so the module joins the second tier rather than the type joining __all__: the tier already carries the same major-bump promise, and the 33-name export list stays as the goal fixed it. The changelog said ApiKey has no __str__. scalars.py defines one returning ApiKey(***). That entry ships inside the sdist and is permanent once uploaded, so it now says both repr and str redact. observability.md called opentelemetry._logs the public logs API, which is the opposite of AGENTS.md and of the premise the deferred import rests on. It now says what is useful: no public alias, so a release inside the accepted range may move it with no notice. AGENTS.md and the changelog also record what the guard and the swallow now cover, which the preceding commits changed.
The program that proves latest-deps re-resolved lived in a heredoc inside ci.yml, where ruff, pyright strict and pylint never saw it. A defect in it would only ever surface as a job failing in CI, and the job it belongs to is the one that is deliberately not a required check. It moves to scripts/resolved_vs_lock.py unchanged in behaviour: it reads the committed lock out of git, because uv sync --upgrade has already rewritten the copy on disk, and it counts the packages the lock carries for another platform rather than looking them up. Strict typing cost two casts, both carrying the comment that proves them.
release.yml has never executed, and the only event that starts it puts a tag on the repository that the ruleset refuses to move or delete. A defect in its first run would burn v0.1.0, and a published version can be yanked but never replaced. workflow_dispatch now runs build, which is the whole gate plus uv build, and stops. publish is gated on github.event_name == 'push', a fact about how the run started rather than an input, a branch or a variable anyone can set, so no dispatched run can reach PyPI. The tag-versus-pyproject check is skipped on a dispatch, where GITHUB_REF_NAME is a branch and there is nothing to publish.
The documentation review found the last two pull requests had moved behaviour without moving its record. The events mode lives on the client now, not on _Config. A ConfigError is raised before the ask span opens, so config can never be an error.type on it, and an alert keyed on that would never fire. The logs signal can be refused or degrade when the installed opentelemetry-api has no logs API, which was stated only in the contributor guide and one docstring, never where a user meets it. Two sentences in the changelog were false and the changelog ships inside the sdist: ApiKey does define __str__, and four of the five scalars are NewType brands the wire validates rather than validated scalars. The second tier now promises one name from guideme.question rather than the module. Naming the module would publish validate, Spec and the concrete question classes to let a caller annotate one type. Tightening the surface test to look for each exported name inside code rather than anywhere in the prose immediately found ModelInfo and fallback documented only in passing, which is what it is for.
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.
The final pre-publish review. Eleven findings, all reproduced against the code before
anything changed, all applied, none rejected. Seven commits, each one finding or one
group of them.
The logs signal could go quiet, twice
AGENTS.mdand the permanent 0.1.0 changelog entry both promise that a release moving theprivate
opentelemetry._logscosts the logs signal and nothing else:import guidemestill works and traces are untouched. Two things made that false.
The guard was narrower than the promise.
resolve_logscaughtImportErroraround thedeferred import, then read
SeverityNumber.INFO,SeverityNumber.WARNand calledget_loggertwice outside it, at module scope throughLOGS = resolve_logs(). Against amodule that imports cleanly and has lost a member:
Every read of that API now happens inside the one guard, which catches
ImportError,AttributeErrorandTypeError. The same probe after:The swallow was wider than its docstring.
Logs._emitwrapped the privateLogRecordconstructor and
logger.emitin oneexcept Exception: pass, while the docstring scopedit to a sink that raises. With a
LogRecordthat no longer takesevent_name, anevents("log")ask produced zero span events, zero log records and no error — the "recordsthat silently go nowhere" the refusal in
GuideBuilder.eventsexists to prevent:resolve_logsnow builds one throwaway record with the five keywords_emituses, so aconstructor change is found where absence is already representable, and
_emitbuilds itsrecord before the
try. The same drift after:The probe still succeeds against the installed 1.44.0, so today's behaviour is unchanged and
the tracing suite still asserts on real exported log records.
The documents disagreed about
guideme.questiondocs/design.mdtold callers to importQuestionfromguideme.question;AGENTS.mdandthe README said the supported surface was
__all__plusguideme.apiandguideme.policy,everything else private. The recorded reason for keeping
Questionout — that no publicsignature returns it — was false:
noul,choose,choose_among,score,score_levelsand all three
.detail()methods returnQuestionsubclasses, andtests/test_live.pyimports it from that module.
Settled by naming
guideme.questionin the second tier, not by growing__all__. The tieralready carries the same major-bump promise, so the contradiction closes without touching the
33-name export list, and the README now documents
Questionwhere a caller meets it, withthe
dictinvariance reason an annotation actually needs it for.The rest
ask's docstringConfigError. It never raises it:choose_amongdoes, at question-construction time. Clause removed,choose_amongsays it from the other side.CHANGELOG.mdApiKey"has no__str__". It has one, returningApiKey(***). That entry ships in the sdist and is permanent once uploaded.docs/observability.mdopentelemetry._logs"the public logs API", the opposite ofAGENTS.mdand of the premise the deferred import rests on.ConfigErrordocstringevents(...)trigger, not the other: an installedopentelemetry-apiwith no logs API.ci.ymlscripts/resolved_vs_lock.py, so ruff, pyright strict and pylint check it. Behaviour proven unchanged both ways: unchanged it prints "nothing in the lock has a newer release today"; with a stalecertifisubstituted into the text it reads out of git, "1 resolved newer than the lock: certifi".api/client.pyrests on httpx dropping the authorization header when a redirect changes origin, and only the same-origin case was tested. A secondHTTPServeron its own port now makes the hop cross origins. The comment names httpx's carve-out: a direct same-hosthttp→httpsupgrade keeps the header deliberately.events(...)release.ymlv0.1.0.workflow_dispatchnow runsbuild— the whole gate plusuv build— andpublishis gated ongithub.event_name == 'push', a fact about how the run started rather than an input, a branch or a variable anyone can set. No dispatched run can reach PyPI.Tests
The ceiling is 43 test functions and the suite was at it. It still is: two more
configuration-mistake parameters and a third redirect arrangement, no new function.
162 passed, 2 deselected, up from 158.
Both new configuration cases were mutation-proved against the unfixed code: with the probe
removed from
resolve_logsthe drift case fails ontelemetry.LOGS is None; with theEVENT_MODEScheck removed the unknown-mode case failsDID NOT RAISE ConfigError.Readings recorded under
## Apply (final)in the assumptions file, including the explicitcorrection of the false rationale in
## Apply (docs).🤖 Generated with Claude Code