fix(observability-map): narrow the required check and the report bot's comment lookup - #4507
Merged
Conversation
…s comment lookup Four findings from Nick's post-merge review of #4455. The report bot found its own comment by marker text with no author predicate, so it silently edited a human's comment that happened to quote the marker. GitHub gates comment editing on write access, not authorship, so this never failed loudly. Matched on .user.login now, the same identity helm-prerelease.yml pins through peter-evans/find-comment. The package's test suite asserted that nobody anywhere in the webapp declares createJWT, signJWT, setImpersonation or updateEnvVars, walking locals and parameters and object keys. Naming a local variable failed a required check with a message that pointed at nothing. Those two negative self-tests move onto a fixture tree, which proves the predicate can fail without holding the webapp's namespace hostage. The positive assertions stay required, because their absence is what rotted the tool before, but each now names the list to edit. The obsmap and internal path filters shared four generic paths, so any lockfile bump ran this suite twice. Dropped from obsmap, where internal already covers them. The test that should have caught it only checked the package's own source path, so it now asserts the filters' set intersection is empty. Editing pr_checks.yml alone no longer fires the obsmap job live, which is the accepted trade-off. The PR comment's footer said nothing here gates the merge. True of the report, misleading now that the suite gates webapp pull requests, so it names both failure directions and where to read the rules.
|
1stvamp
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (21)
🔇 Additional comments (2)
WalkthroughThe observability-map tests now scan caller-provided source and route roots with fixture coverage for declarations and route segments. Report comment lookup requires the marker and 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The fixture asserted the local and the member declaration forms but not the function declaration, so deleting that branch of the walker left the fixture test green and failed only the live-tree assertions the fixture exists to replace. Assert `helper` too, so each branch is pinned by its own name. Revert-confirmed: removing the branch now fails the fixture test.
1stvamp
marked this pull request as ready for review
August 5, 2026 19:21
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nicktrn
approved these changes
Aug 5, 2026
… timings Updating the workflow's entry count left three prose figures behind: the README said 44 rewrites applied and 43 defended, and both the README and INTERNALS said four and a half minutes. The corpus has 53 entries. The worse one was not a count. The README claimed one hole is open when KNOWN_GAPS has two, so the document under-reported what the corpus cannot defend, which is the one thing it has to be honest about. The second gap, dead-conjunction-instanceof-if, is now written out beside the first with its reason. Timings measured rather than scaled: the corpus step is about 1.7 minutes on CI's runner and about 8 on a laptop, so the earlier five-minute figure in the workflow was wrong too. Replaced the single numbers with the range, since an exact figure is machine-dependent and nothing asserts it.
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
2 participants
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.
Findings addressed
.user.login == "github-actions[bot]", the same identityhelm-prerelease.ymlalready pins.webappSymbols.test.tsasserted that nobody anywhere inapps/webapp(walking locals, params, object keys) declares names likecreateJWT/updateEnvVars, so an unrelated PR naming a local variable failed a required check with a message pointing at nothing. Those negative self-tests move onto a package-owned fixture tree; the positive resolution assertions stay required (their absence rotted the tool before) but now name the list to edit.obsmapandinternalpath filters shared four generic paths (package.json, both lockfiles,pr_checks.yml), so any lockfile bump ran the observability-map suite in both jobs. Dropped fromobsmap(whereinternalalready covers them). The test that should have caught it only checked the package's own source path; it now asserts the two filters' path intersection is empty.Review
Two adversarial reviewers ran over the diff; both findings were verified and fixed: a hollow fixture assertion (a shared name satisfied either walker branch — now one name per declaration form, revert-confirmed) and a filter-intersection test that could be fooled by apostrophes in comment prose (now strips comment lines first). Full package suite green (877 passed), typecheck and format clean.