Skip to content

Name the package each admin action routes to as its owner - #847

Merged
jmrplens merged 1 commit into
plan-l9from
plan-l10
Sep 20, 2026
Merged

jmrplens merged 1 commit into
plan-l9from
plan-l10

Conversation

@jmrplens

@jmrplens jmrplens commented Sep 20, 2026

Copy link
Copy Markdown
Owner

adminspecs declared itself the owner of all ninety-two instance administration actions, and it is the one package among them that never issues a request: every handler those actions route to lives in one of the twenty-three domain packages the route names, and that is the package the test transport writes into the request inventory.

The owner is a join key rather than a label. R-PATH's observation check joins the catalog to the inventory on that name and on nothing else, so ninety-two actions could be joined to no recording at all, and a declaration in declarations.go existed only to keep the gate quiet about it. adminOptions now takes the owner beside the route, each one a constant naming the package whose handler that route calls, and the declaration is retired. The table is left in place and empty, which is the healthy state for it rather than an unfinished one.

The owner constants are not trusted on their own. TestActionSpecs_OwnerPackage_IsThePackageTheHandlerComesFrom holds each of the ninety-two against the package its route's input type comes from, which is a fact of the route rather than a second copy of the table: every admin handler takes the input type its own package declares, so a copy-pasted constant fails instead of landing. That is the one error R-PATH itself cannot catch, since an owner naming a real package and the wrong one reads exactly like a right one.

R-PATH now observes 1082 of 1082 catalog actions, with no silent package and no declaration. The sharper join immediately earned its keep in R-PAGE, which reports rather than gates: admin.system_hook_list moved out of the not-asked-about count and into the findings, because systemhooks records exactly one paginated endpoint, GET /hooks, which is the endpoint that action calls. It answers with a bare array of hooks and accepts no page or per_page, which is the user.list_impersonation_tokens class R-PAGE was built for and which the coarse owner had been hiding. Left as a finding, deliberately: there is no declaration category for a list GitLab pages and we do not.

(cherry picked from commit 2a44dd1214acae9cf3ee0db0c5419751dab4ff22)

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @jmrplens, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 10 hours and 48 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Warning

Review limit reached

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: jmrplens/gitlab-mcp-server/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 9c9032a0-80a9-4263-848c-a43acd66f7a3

📥 Commits

Reviewing files that changed from the base of the PR and between cea90ce and d6a1b9f.

📒 Files selected for processing (6)
  • CLAUDE.md
  • cmd/audit_1to1/internal/paths/declarations.go
  • cmd/audit_1to1/internal/paths/doc.go
  • cmd/internal/requestinventory/coverage.go
  • internal/tools/adminspecs/action_specs.go
  • internal/tools/adminspecs/action_specs_test.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: jmrplens/gitlab-mcp-server/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 76d76227-a547-47ed-9076-dddd5f7ea4a6

📥 Commits

Reviewing files that changed from the base of the PR and between c3c2cbb and cea90ce.

📒 Files selected for processing (6)
  • CLAUDE.md
  • cmd/audit_1to1/internal/paths/declarations.go
  • cmd/audit_1to1/internal/paths/doc.go
  • cmd/internal/requestinventory/coverage.go
  • internal/tools/adminspecs/action_specs.go
  • internal/tools/adminspecs/action_specs_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Documentation

    • Updated audit and coverage documentation to reflect revised ownership of administrative actions and current audit findings.
  • Improvements

    • Administrative actions are now attributed to the domain packages that handle their routes, improving request coverage and ownership accuracy.
  • Tests

    • Added validation ensuring each administrative action has a valid owner matching its handler package.

Walkthrough

Admin action specifications now assign each action to the domain package named by its route. Tests verify the ownership. Audit declarations and documentation reflect the reassignment of all 92 actions.

Changes

Admin owner alignment

Layer / File(s) Summary
Domain owner wiring
internal/tools/adminspecs/action_specs.go
The action specifications define domain owner constants, pass them through spec builders, and set OwnerPackage from the supplied owner.
Owner validation
internal/tools/adminspecs/action_specs_test.go
Tests verify that every action has input type metadata and that OwnerPackage matches the input type package.
Audit classification and documentation
cmd/audit_1to1/internal/paths/declarations.go, cmd/audit_1to1/internal/paths/doc.go, cmd/internal/requestinventory/coverage.go, CLAUDE.md
The adminspecs silent-owner declaration is removed. Comments and audit counts describe the reassigned ownership and resulting findings.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 5 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description provides a detailed technical summary, but it does not use the required template sections. It omits the related issue, change type, explicit changes list, testing steps, migration note… Rewrite the description using the repository template. Add the required headings, provide the related issue or explain its absence, select the change type, list the changes, document concrete test commands and results, state migration notes…
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the primary change: assigning each admin action to the package that handles its route.
Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 5 files. (1 skipped: 1 unsupported.)

Full details: Description check

Explanation

The description provides a detailed technical summary, but it does not use the required template sections. It omits the related issue, change type, explicit changes list, testing steps, migration notes, checklist status, and applicable logs or screenshots.

Resolution

Rewrite the description using the repository template. Add the required headings, provide the related issue or explain its absence, select the change type, list the changes, document concrete test commands and results, state migration notes as N/A if applicable, and complete the relevant checklist items.

✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the tooling The audit and generator commands under cmd/, and the Makefile targets that run them label Sep 20, 2026
@jmrplens
jmrplens added this pull request to stack #863 September 20, 2026 10:16
@jmrplens jmrplens added this to the 3.1.0 milestone Sep 20, 2026
@github-actions github-actions Bot added the v3.1.0 Targeted at the 3.1.0 release label Sep 20, 2026
@jmrplens
jmrplens force-pushed the plan-l10 branch 3 times, most recently from bf61305 to 59534ed Compare September 20, 2026 14:43
@jmrplens
jmrplens removed this pull request from stack #863 September 20, 2026 19:40
@jmrplens
jmrplens added this pull request to stack #866 September 20, 2026 19:40
`adminspecs` declared itself the owner of all ninety-two instance
administration actions, and it is the one package among them that never
issues a request: every handler those actions route to lives in one of
the twenty-three domain packages the route names, and that is the
package the test transport writes into the request inventory.

The owner is a join key rather than a label. R-PATH's observation check
joins the catalog to the inventory on that name and on nothing else, so
ninety-two actions could be joined to no recording at all, and a
declaration in `declarations.go` existed only to keep the gate quiet
about it. `adminOptions` now takes the owner beside the route, each one
a constant naming the package whose handler that route calls, and the
declaration is retired. The table is left in place and empty, which is
the healthy state for it rather than an unfinished one.

The owner constants are not trusted on their own.
`TestActionSpecs_OwnerPackage_IsThePackageTheHandlerComesFrom` holds
each of the ninety-two against the package its route's input type comes
from, which is a fact of the route rather than a second copy of the
table: every admin handler takes the input type its own package
declares, so a copy-pasted constant fails instead of landing. That is
the one error R-PATH itself cannot catch, since an owner naming a real
package and the wrong one reads exactly like a right one.

R-PATH now observes 1082 of 1082 catalog actions, with no silent package
and no declaration. The sharper join immediately earned its keep in
R-PAGE, which reports rather than gates: `admin.system_hook_list` moved
out of the not-asked-about count and into the findings, because
`systemhooks` records exactly one paginated endpoint, `GET /hooks`,
which is the endpoint that action calls. It answers with a bare array of
hooks and accepts no `page` or `per_page`, which is the
`user.list_impersonation_tokens` class R-PAGE was built for and which
the coarse owner had been hiding. Left as a finding, deliberately: there
is no declaration category for a list GitLab pages and we do not.

(cherry picked from commit 2a44dd1214acae9cf3ee0db0c5419751dab4ff22)
@sonarqubecloud

Copy link
Copy Markdown

@jmrplens
jmrplens merged commit 8f24d0d into main Sep 20, 2026
37 of 41 checks passed
@jmrplens
jmrplens deleted the plan-l10 branch September 20, 2026 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tooling The audit and generator commands under cmd/, and the Makefile targets that run them v3.1.0 Targeted at the 3.1.0 release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant