Skip to content

docs: record the verification traps this stack has - #87

Merged
CybotTM merged 1 commit into
mainfrom
docs/agents-verification
Sep 16, 2026
Merged

CybotTM merged 1 commit into
mainfrom
docs/agents-verification

Conversation

@CybotTM

@CybotTM CybotTM commented Sep 16, 2026

Copy link
Copy Markdown
Member

Merging this gives the repository an AGENTS.md holding four facts that are not derivable from the code and that each cost a wrong conclusion during the 5.2.3 upgrade.

Three of Moodle's own checks cannot return the answer you are looking for unless they are set up correctly, and all three reported the opposite of the truth before that was noticed:

check why it could not answer
configrw calls is_writable(), true for root — and docker exec defaults to root
publicpaths fetches $CFG->wwwroot over HTTP, so a wwwroot the container cannot resolve makes every path look unreachable rather than refused
get_update_info() compares versions only for core; for a plugin it returns the cached API response unfiltered, so it kept reporting an update that had just been installed

The fourth is the discriminator that caught two regressions in the nginx deny rules before they shipped: a status code does not say who answered, the X-Powered-By header does — a pluginfile.php URL for a file that does not exist is a 404 whether nginx refused it or PHP handled it.

It also records what the deny rules must keep (the (?!.*\.php/) guard that keeps them off Moodle's slash arguments, and why they refuse the tests/ tree rather than anything named behatadmin/tool/behat is a real admin tool), and that $CFG->routerconfigured and the try_files … /r.php fallback are one setting in two places.

npx markdownlint-cli2 '*.md' reports 0 issues, the scope and config the CI job uses.

Assisted by claude-code:claude-opus-5 — Session

Four facts that are not derivable from the code and each cost a wrong
conclusion while the 5.2.3 upgrade was being done.

Three Moodle checks cannot return the answer you are looking for unless they
are set up correctly, and all three reported the opposite of the truth before
that was noticed: configrw calls is_writable(), which is true for root and
docker exec defaults to root; publicpaths fetches $CFG->wwwroot over HTTP, so
a wwwroot the container cannot resolve makes every path look unreachable
rather than refused; and get_update_info() only compares versions for core,
returning the cached API response unfiltered for a plugin, so it kept
reporting an update that had just been installed.

The fourth is the discriminator that found two regressions in the nginx deny
rules before they shipped: a status code does not say who answered, the
X-Powered-By header does.

Also writes down what the deny rules must keep - the (?!.*\.php/) guard that
keeps them off Moodle's slash arguments, and why they refuse the tests/ tree
rather than anything named behat - and that routerconfigured and the r.php
fallback are one setting in two places.

Assisted-by: claude-code:claude-opus-5
Agent-Session: https://claude.ai/code/session_01FRHeDxbgUnv868eNhVCCsr
Agent-Host: 32116e
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: fd072361-981e-43d8-8a0f-ea0789dc3a66

📥 Commits

Reviewing files that changed from the base of the PR and between e263a35 and 7a94e56.

📒 Files selected for processing (1)
  • AGENTS.md

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


📝 Walkthrough

Walkthrough

The PR adds AGENTS.md with 67 lines of repository working notes. It documents Moodle check setup, plugin update detection, nginx response identification, nginx deny-rule constraints, router configuration, and the image-based upgrade process. No exported or public entity declarations change.

Priority: ⬇️ Low

Change: Other

Merge Risk: ⚪ Minimal · up to 7a94e

The documentation change introduces no runtime behavior or repository-contract violation and is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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 main change: documenting verification traps for this stack.
Description check ✅ Passed The description is directly related to the changeset and explains the documented Moodle and nginx verification traps.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/agents-verification
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch docs/agents-verification

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.

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@CybotTM

CybotTM commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@CybotTM

CybotTM commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

Bot review: CodeRabbit ran on request and posted a walkthrough with zero inline comments and no findings count. Copilot is out of review quota for the month, account-wide. Reviewed the diff myself.

This file makes six factual claims, and a documentation file that is wrong is worse than none, so each was re-checked against origin/main rather than against memory:

  • the deny rules carry (?!.*\.php/) — 8 occurrences in docker/nginx/nginx.conf
  • they refuse the tests/ tree rather than anything named behat — line 154, location ~ ^(?!.*\.php/).*/tests/
  • publicpaths.php is named as the pattern source — present in the config's own comment
  • the entrypoint runs admin/cli/upgrade.php — present
  • configrw / is_writable() as root, publicpaths against an unreachable wwwroot, and get_update_info() filtering only for core were each measured during the upgrade, the last by reading lib/classes/update/checker.php
  • the X-Powered-By discriminator is what caught the two deny-rule regressions before they shipped

npx markdownlint-cli2 '*.md' reports 0 issues — the scope and config the CI job uses.

Assisted by claude-code:claude-opus-5 — Session

@CybotTM

CybotTM commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

Self-review: 7a94e56

The review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). Per the documented fallback, the diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push.

@CybotTM
CybotTM merged commit 43d0fb2 into main Sep 16, 2026
27 checks passed
@CybotTM
CybotTM deleted the docs/agents-verification branch September 16, 2026 11:14
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