Repository navigation
ci: stop new direct reads of cached telemetry - #1677
Conversation
…ackage A reading stays in telemetry.Store after its driver goes stale, offline or faulted. Every direct reader of RawW, SmoothedW, ReadingsByType or ReadingsByDriver must check driver health itself, and review caught the missing check in #36, #85, #310, #647, #664 and #740. The check counts those reads per file outside go/internal/telemetry and fails when a pull request adds one. Existing reads (147 today) may only go down. The error names the health-gated accessors to use instead. On the merged commits of #36, #85, #310 and #647 it fails.
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6879c9c03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| READ = re.compile(r"\.(?:RawW|SmoothedW)\b|\.(?:ReadingsByType|ReadingsByDriver)\s*\(") | ||
| EXCEPTION = re.compile( | ||
| r"//\s*telemetry-read-ok:\s*(?P<reason>[^;]+);\s*" | ||
| r"expires (?P<date>\d{4}-\d{2}-\d{2});\s*approved-by (?P<owner>@?[\w.-]+)\s*$" |
There was a problem hiding this comment.
For any new direct read annotated with approved-by - or approved-by ..., this character class matches, and the exception is treated as valid based only on its reason and date. CI therefore suppresses the read even though no named owner exists to review it; require the approver value to contain at least one alphanumeric character before accepting the exception.
Useful? React with 👍 / 👎.
…1679) * docs: propose a stepwise plan to make FTW safe for agents to change Agents write most of FTW, and review bots raised 1,150 findings on 573 of their pull requests. This plan groups those findings into recurring mistake classes, proposes a structural fix for each at the highest level that works, backtests each fix against the history, and orders the fixes into independently mergeable steps. Steps 1, 2 and 4 are draft PRs #1676, #1677 and #1678. tools/agent-pitfalls holds the script and the labels so a reviewer can rerun or spot-check every number. * docs: keep the pitfall lever under docs/ so the PR stays docs-only * docs: distinguish model estimates from executable evidence Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> --------- Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Reject per-file increases in four cached-telemetry read forms outside
go/internal/telemetry:.RawW,.SmoothedW,ReadingsByType(andReadingsByDriver(. The script now counts every occurrence, including multiple reads on one line. It uses a POSIX regex for Git and Python for counting, so Apple Git and Linux agree.An exception must include a reason, a valid unexpired date and a named approver on the same line:
// telemetry-read-ok: <reason>; expires YYYY-MM-DD; approved-by <name>The named owner must review the exception in the PR; the script checks its form and date. Git errors fail the check.
Validation:
dispatch.go4 → 6), feat(pvmodel): anchor near-future predictions to live telemetry #85 (pvmodel/service.go3 → 6), feat(control): DC-link protective PV curtail — pre-empt Ferroamp fault #310 (dispatch.go23 → 32) and fix: harden optimizer and energy runtime recovery #647 (main.go23 → 27), each against its first parent.A count increase is evidence of a new read, not proof of a stale-data bug. This ratchet permits count-neutral replacements and does not supply S3's typed freshness guarantee. The earlier 42-of-1,423 cost estimate used the old line counter and has not been rerun for the revised check.
Part of #1679. CI-only change; no changeset. Remains draft for the owner's merge decision.