Skip to content

docs(agents): capture CSRF and e2e-cleanup learnings from #652 - #663

Merged
CybotTM merged 2 commits into
mainfrom
docs/agents-csrf-learnings
Aug 26, 2026
Merged

docs(agents): capture CSRF and e2e-cleanup learnings from #652#663
CybotTM merged 2 commits into
mainfrom
docs/agents-csrf-learnings

Conversation

@CybotTM

@CybotTM CybotTM commented Aug 26, 2026

Copy link
Copy Markdown
Member

Documentation follow-up from the #652 investigation (retro outcome). Three additions to internal/web/AGENTS.md, each a fact that cost real debugging time because it lived only in test comments and PR text:

  • The "CSRF errors" pointer now names the actual failure class: a persistent 403 "CSRF token validation failed" means the POST carries no csrf_token; JS-built forms read the session token from data-csrf on main[data-bulk-scope], server-rendered forms embed a hidden input.
  • The test-infrastructure section states that setupFullTestApp registers routes without the CSRF middleware — the exact coverage gap that let Potential Bug: CSRF token validation failed #652 ship — and points to setupCSRFBulkTestApp for exercising the production chain.
  • The integration-test list gains the cleanup rule: delete seeded entries through the app, not via direct ldapdelete — the 30s directory cache keeps a ghost that poisons later tests (seen in CI as a cascading failure of TestAddRemoveGroupMembership).

Two pre-existing over-length bullets in the same section are re-wrapped because the markdownlint hook (MD013, 120 columns) blocks any commit touching the file.

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

Three additions to internal/web/AGENTS.md, all learned the hard way in
the #652 investigation:

- "CSRF errors" pointer now names the actual failure class: a
  persistent 403 means the POST carries no token; JS-built forms read
  it from data-csrf on main[data-bulk-scope].
- Test-infrastructure section states that setupFullTestApp strips the
  CSRF middleware (the gap that let #652 ship) and points to
  setupCSRFBulkTestApp for the real chain.
- Integration-test list gains the cleanup rule: delete through the
  app, not via direct ldapdelete — the 30s cache keeps a ghost that
  poisons later tests.

Two pre-existing over-length bullets in the same section are
re-wrapped because the markdownlint hook (MD013, 120 cols) blocks any
commit touching the file.

Assisted-by: claude-code:claude-fable-5
Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

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

Scanned Files

None

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Aug 26, 2026
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.21%. Comparing base (b43afd9) to head (486c406).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #663   +/-   ##
=======================================
  Coverage   74.21%   74.21%           
=======================================
  Files          36       36           
  Lines        3157     3157           
=======================================
  Hits         2343     2343           
  Misses        814      814           
Flag Coverage Δ
e2e 67.56% <ø> (ø)
unittests 74.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Qualify the test-file reference: the cleanup pattern lives in
  internal/e2e/bulk_toolbar_csrf_test.go, not next to the near-namesake
  bulk_csrf_test.go in internal/web/.
- "session token" -> "per-session CSRF token" so nobody conflates it
  with the auth session cookie.
- internal/AGENTS.md gains a pointer to the e2e cleanup rule: agents
  working in internal/e2e/ load that file, not internal/web/AGENTS.md,
  under closest-file precedence.
- Pre-existing markdownlint debt in the touched file settled because
  the hook blocks any commit otherwise: over-length bullets re-wrapped
  (MD013), two bare directory-tree fences get a text language (MD040).

Assisted-by: claude-code:claude-fable-5
Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
@CybotTM
CybotTM marked this pull request as ready for review August 26, 2026 06:09
Copilot AI lite review requested due to automatic review settings August 26, 2026 06:09
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

@CybotTM
CybotTM added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit ff65290 Aug 26, 2026
33 of 34 checks passed
@CybotTM
CybotTM deleted the docs/agents-csrf-learnings branch August 26, 2026 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants