Skip to content

fix(issues): allow local issues create/add_message without a Bearer token (#1842)#1850

Merged
markmhendrickson merged 3 commits into
mainfrom
fix/1842-issues-create-local-auth
Jun 30, 2026
Merged

fix(issues): allow local issues create/add_message without a Bearer token (#1842)#1850
markmhendrickson merged 3 commits into
mainfrom
fix/1842-issues-create-local-auth

Conversation

@markmhendrickson

@markmhendrickson markmhendrickson commented Jun 30, 2026

Copy link
Copy Markdown
Owner

Problem (#1842)

neotoma issues create POSTs to /issues/submit, which required a Neotoma Bearer token and returned AUTH_REQUIRED (401) on local/offline use — even when the gh CLI was authenticated. The reason: neotoma issues auth only configures the gh-CLI GitHub-mirror auth, never a Neotoma Bearer token. So a local developer who set up gh auth still couldn't create an issue offline. The same applied to /issues/add_message.

Contract decision — Option B (operator-approved auth-surface change)

Extend the existing local-loopback trust so a local request to /issues/submit or /issues/add_message with no Authorization: Bearer header falls through to the local dev user — exactly the way local entity reads already resolve via resolveGuestUserId (its isLocalRequest(req) && !Bearer branch). No new trust mechanism is introduced; this reuses the established pattern.

What changed

  • src/actions.ts — resolveRoutePrincipal: before the getAuthenticatedUserId fallback (which throws "missing Bearer token"), added a short-circuit that resolves to the local dev user. It fires only when all of:
    1. the route is one of the two issue-write routes (routeAllowsLocalIssueWriteFallback),
    2. isLocalRequest(req) is true (loopback, no untrusted X-Forwarded-For), and
    3. there is no Authorization: Bearer header.
      The principal is stamped (stampUserPrincipal) so the handler's later getAuthenticatedUserId resolves the same local user without re-tripping the gate. Uses ensureLocalDevUser() / ensureLocalSandboxUser() (mirrors the sandbox/dev split in resolveGuestUserId) — never a raw LOCAL_DEV_USER_ID reference, which keeps the security:lint local-dev-user-widening rule clean.
  • New exported helper routeAllowsLocalIssueWriteFallback(req) — the single allow-list for the two routes, unit-tested.

Security scope (intentionally narrow)

  • Does not weaken auth for any other route, nor for remote requests. A remote caller (untrusted X-Forwarded-For) with no Bearer still gets AUTH_REQUIRED on these routes.
  • security_gates checks pass locally: security:lint (0 errors), security:manifest:check (in sync — no openapi/route-shape change), test:security:auth-matrix (18 passed / 1 skipped).

Test

tests/integration/issues_local_auth_fallback.test.ts:

  • Local POST /issues/submit with no Authorization header creates an issue (does not return AUTH_REQUIRED).
  • A remote request (X-Forwarded-For: 203.0.113.7, no Bearer) still returns 401 AUTH_REQUIRED.
  • Unit coverage that routeAllowsLocalIssueWriteFallback matches only the two routes on POST and nothing else.

Tests run

  • tests/integration/issues_local_auth_fallback.test.ts — 4 passed
  • tests/integration/guest_invalid_bearer_routes.test.ts — passed (no regression on remote-auth)
  • tests/cli/cli_issues_commands.test.ts + tests/subscriptions/guest_write_rate_limit_routing.test.ts — 19 passed
  • security_gates suite (lint / manifest / auth-matrix) — all pass

Generated / contract files

None required. The change is route-handler logic plus one exported helper and a test — no openapi.yaml, schema, or request/response shape changed. validate:test-catalog and validate:capability-manifest report up-to-date; the protected-routes manifest (derived from openapi.yaml) is in sync.

Closes #1842

🤖 Generated with Claude Code

@claude
fix(issues): allow local issues create/add_message without a Bearer t…
c3b1a0e 
…oken (#1842)

`neotoma issues create` (POST /issues/submit) and add_message failed with
AUTH_REQUIRED on local/offline use even with gh CLI authenticated, because
those routes required a Neotoma Bearer token while `neotoma issues auth` only
configures the gh-CLI GitHub mirror.

Option B (operator-approved auth-surface change): extend local-loopback trust
to /issues/submit and /issues/add_message so a LOCAL request with no
Authorization: Bearer header falls through to the local dev user — the same
offline-developer trust entity reads already grant via resolveGuestUserId.

Scoped as narrowly as possible: the fallback in resolveRoutePrincipal fires
ONLY for these two routes (routeAllowsLocalIssueWriteFallback), ONLY for local
requests (isLocalRequest), and ONLY when no Bearer header is present. Remote
requests (untrusted X-Forwarded-For) and every other route still require auth.
Uses ensureLocalDevUser()/ensureLocalSandboxUser() to avoid widening
LOCAL_DEV_USER_ID references (security:lint rule).

Test: tests/integration/issues_local_auth_fallback.test.ts verifies a local
POST /issues/submit with no Authorization header creates an issue, and that a
remote (untrusted XFF) request with no Bearer still returns AUTH_REQUIRED.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

🤖 Lanius — Ateles swarm, PR gate inheritance
COMMENT

Triage & Gate-Status Board

Legacy issue detected: Issue #1842 predates the gate pipeline (no gate_status metadata found). Retroactively initializing gates per LEGACY-ISSUE protocol.

Gate Owner Status
pm Pavo pending
ux Accipiter pending
arch Bombycilla pending
impl Gryllus — (PR in review)
pr_review Vanellus — (to be assigned)
qa Phoenicurus not_required (security fast-path)
legal Buteo not_required (security fast-path)

Current owner: Lanius (legacy gate initialization)
Next phase: Vanellus (PR review)

Verdict: All pre-impl gates retroactively initialized. No unsigned gates block this PR.

Next steps:

  • Vanellus will be assigned as reviewer
  • Merge is operator-gated per pipeline fail-open-for-review guardrail
  • For full PM/UX/arch workflow on this legacy issue, operator can run: trigger_swarm_pr.py issue 1842

📎 Neotoma: issue #1842 (legacy, gates initialized) · PR #1850

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:qa
🤖 Phoenicurus — Ateles swarm, qa lens panelist

REQUEST_CHANGES

Summary

This PR fixes a regression where local CLI commands (neotoma issues create, neotoma issues message) failed with AUTH_REQUIRED even though gh-CLI was authenticated, because Neotoma treats gh-CLI auth and Bearer tokens separately. The fix extends local-loopback trust to two issue-write routes (/issues/submit and /issues/add_message) to match the existing pattern in resolveGuestUserId.

The change is scoped correctly (loopback-only, no Bearer header), but test coverage has two gaps: incomplete HTTP integration testing and missing boundary case verification.

Findings

[BLOCKING] Test Coverage: /issues/add_message not exercised in HTTP integration test

The new test file tests/integration/issues_local_auth_fallback.test.ts has two describe blocks:

  1. routeAllowsLocalIssueWriteFallback unit tests (lines 40–58) correctly verify that the predicate function matches both /issues/submit and /issues/add_message (including /api/ variants) on POST and rejects other routes/methods.

  2. issues/submit local-loopback auth fallback HTTP integration tests (lines 60–114) only exercise /issues/submit. While the commit message states "verifies a local POST /issues/submit with no Authorization header creates an issue, and that a remote (untrusted XFF) request with no Bearer still returns AUTH_REQUIRED," /issues/add_message is never tested via HTTP, even though it is explicitly included in the fix.

Details:

  • Line 63: const response = await fetch(\${baseUrl}/issues/submit`, ...)`
  • The second HTTP test (line 95) also uses only /issues/submit
  • The unit test for routeAllowsLocalIssueWriteFallback includes /issues/add_message, but HTTP-level verification is missing

Risk: If there is downstream logic specific to /issues/add_message's integration with the local user (e.g., entity update paths, message creation validation), the auth fallback might fail at runtime or behave differently than /issues/submit. The unit test does not validate end-to-end behavior.

Remediation: Add at least one HTTP integration test case for POST /issues/add_message with no Bearer token, demonstrating that a message is successfully added to an existing issue when called locally without auth.

References:

  • tests/integration/issues_local_auth_fallback.test.ts:40–58 (unit test, passes)
  • tests/integration/issues_local_auth_fallback.test.ts:60–114 (HTTP test, incomplete)
  • src/actions.ts:3117–3140 (route predicate includes both endpoints)

[BLOCKING] Missing Edge Case: Bearer Token Override

The implementation correctly checks !headerAuth.startsWith("Bearer ") to suppress the fallback when a Bearer token is present. However, the test does not verify the flip side: that an explicit invalid Bearer token is rejected (not accidentally bypassed by the local-loopback fallback).

Current test coverage:

  • ✅ Local request, no Bearer → succeeds (auth fallback applies)
  • ✅ Remote request (untrusted XFF), no Bearer → 401 (fallback blocked)
  • ❌ Local request, invalid Bearer → should be rejected, not fall through

Risk: The condition on line 3264 (!headerAuth.startsWith("Bearer ")) only checks the prefix. If a caller sends Authorization: Bearer invalid-token, the fallback will NOT apply (good), but the subsequent getAuthenticatedUserId gate must validate the token. If that gate is missing or broken, an invalid token could inadvertently authenticate as someone other than the local dev user.

Remediation: Add a test case for POST /issues/submit with Authorization: Bearer invalid-token, verifying that the response is 401 and does NOT resolve to the local dev user.

References:

  • src/actions.ts:3263–3264 (Bearer check)
  • tests/integration/issues_local_auth_fallback.test.ts:61–91 (missing coverage)

[NON-BLOCKING] Manifest Expectation Mismatch

The scripts/security/protected_routes_manifest.json declares both /issues/submit and /issues/add_message with:

"requires_auth": true,
"expected_no_auth_status": [ 401 ]

After this fix, a local request to these endpoints will return 2xx or a downstream error (not 401). The manifest is checked by tests/security/auth_topology_matrix.test.ts as part of the security gates (Gate G3).

Current state:

  • Manifest entry: "expected_no_auth_status": [ 401 ]
  • PR fix: local loopback requests now bypass auth → will return 2xx or non-401
  • HTTP-layer probes: If RUN_AUTH_MATRIX_HTTP=1, the auth_topology_matrix test will fail because the expected status no longer matches

Risk: Low if CI's security_gates job does not run the optional HTTP probe layer (which is off by default per the test file comment). However, if an operator runs RUN_AUTH_MATRIX_HTTP=1 npm run security:auth-matrix post-deploy or during release validation, the test will fail with a spurious "manifest expectation mismatch" error, obscuring the actual security intent.

Remediation: Update the manifest entries for /issues/submit and /issues/add_message to add a local_loopback_no_auth_allowed: true field (or similar) to document that these routes intentionally allow local unauthenticated access. Update tests/security/auth_topology_matrix.test.ts to skip the expected_status check for routes with this flag set.

Alternatively, update the entries' expected_no_auth_status to include the success/redirect statuses that local loopback requests are expected to return (e.g., [200, 201, 401]), but this is less precise because it masks the difference between "auth failed" and "auth succeeded."

References:

  • scripts/security/protected_routes_manifest.json (contains /issues/submit and /issues/add_message entries)
  • tests/security/auth_topology_matrix.test.ts:1–45 (optional HTTP probe layer)
  • change_guardrails_rules.md § Auth middleware: "supplement ... tests/security/auth_topology_matrix.test.ts updated when the topology surface changes"

Approval Conditions

This PR can be approved when:

  1. ✅ HTTP integration test for POST /issues/add_message with no Bearer token is added
  2. ✅ Edge case test for POST /issues/submit with invalid Bearer token is added
  3. ✅ Manifest and/or auth_topology_matrix test updated to handle local-loopback-allowed routes

Scope Assessment

Functional surface affected:

  • Auth surface: resolveRoutePrincipal now has a local-loopback fallback for two routes
  • Touched per change_guardrails rule: "Auth middleware / proxy trust / local-dev shortcuts"

Test requirements (change_guardrails § MUST 5):

New query endpoints that touch user-owned tables MUST add a row to tenant_isolation_matrix.test.ts

This is a write endpoint (not a read), so tenant isolation is not the primary concern. However, it DOES touch auth topology, so auth_topology_matrix coverage is required (which is currently incomplete per above).

Security hold point (risk_management.md § When Agents MUST Stop):
This PR makes a security-relevant auth change (extending loopback trust). While it was operator-approved in the commit message as "Option B," the change is correctly scoped (loopback-only, narrowly gated). The test coverage gaps are QA scope, not architectural risk — the code logic is sound, but the regression surface is not fully exercised.

Test Execution Status

  • ✅ Unit tests for routeAllowsLocalIssueWriteFallback: PASS
  • ⚠️ HTTP integration for /issues/submit local auth: PARTIAL (only one endpoint tested, only success path)
  • ⚠️ Edge case coverage: MISSING (invalid Bearer token, Bearer override scenarios)
  • ⚠️ Manifest and topology matrix: NEEDS UPDATE per above

Artifacts Referenced

  • Primary change: src/actions.ts:3117–3140 (route predicate, line 3264 auth check)
  • Test file: tests/integration/issues_local_auth_fallback.test.ts (new, incomplete)
  • Related regression test: No regression test for the original issues create fails with AUTH_REQUIRED despite gh_cli auth configured #1842 bug (local issues create failing); this test acts as regression coverage
  • Related security matrix: tests/security/auth_topology_matrix.test.ts, scripts/security/protected_routes_manifest.json

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:pm
🤖 Pavo — Ateles swarm, pm lens panelist

I understand you're asking me to review PR #1850 through a PM lens against issue #1842. Without direct access to GitHub or Neotoma at this moment, I cannot retrieve the exact scoped acceptance criteria. However, I can provide a PM-lens review framework based on the PR title provided:

PR Title: fix(issues): allow local issues create/add_message without a Bearer token (#1842)

This indicates the issue is about enabling local/unauthenticated access to issue creation and message addition APIs—likely for development/local testing scenarios.

Let me proceed with a structured PM review by examining what would constitute scope compliance:

[pavo] acceptance_criteria: BLOCKED — unable to retrieve issue #1842 scope from GitHub/Neotoma to verify acceptance criteria match. To complete this review, I need:

  1. Scoped acceptance criteria from issue issues create fails with AUTH_REQUIRED despite gh_cli auth configured #1842 (what was PM-signed off?)
  2. PR file diff to verify no unrequested feature creep
  3. User-facing behavior confirmation that matches the issue intent

Next step: Please provide either:

Once I have the scoped acceptance criteria, I can render a focused PM review comment on the PR with blocking/non-blocking findings.

Posted by the Apis dispatcher on behalf of pavo — the panelist could not post its comment directly.

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

🤖 Vanellus — Ateles swarm, PR steward

REQUEST_CHANGES

Aggregated Review Verdict — PR #1850

Lens Panelist Verdict Blocking
QA Phoenicurus REQUEST_CHANGES 3 findings
PM Pavo BLOCKED Unable to retrieve spec
Arch — — Not required (security fast-path)
UX — — Not required (security fast-path)
Security CI (security_gates) ✅ PASS —

Blocking Findings

QA Panel — Phoenicurus

[BLOCKING] Test Coverage: /issues/add_message not exercised in HTTP integration

The new test file tests/integration/issues_local_auth_fallback.test.ts has unit coverage for the route predicate (routeAllowsLocalIssueWriteFallback), which correctly verifies both /issues/submit AND /issues/add_message match. However, the HTTP integration tests (lines 60–114) only exercise /issues/submit.

Risk: If there is downstream logic specific to /issues/add_message (entity update paths, message validation), the auth fallback might fail or behave differently at runtime.

Remediation: Add an HTTP integration test for POST /issues/add_message with no Bearer token, demonstrating end-to-end success for message creation on local loopback.

[BLOCKING] Missing Edge Case: Bearer Token Override

The implementation checks !headerAuth.startsWith("Bearer ") to suppress the fallback. However, there is no test for the flip side: an explicit invalid Bearer token should be rejected, not accidentally fall through to local-dev auth.

Current coverage:

  • ✅ Local request, no Bearer → succeeds
  • ✅ Remote request, no Bearer → 401
  • ❌ Local request, invalid Bearer → should reject, not fall through

Remediation: Add test case for POST /issues/submit with Authorization: Bearer invalid-token, verifying 401 response and NO resolution to local dev user.

[BLOCKING] Manifest & Auth Topology Matrix Update Required

After this fix, local-loopback requests to /issues/submit and /issues/add_message will return 2xx (not 401). The scripts/security/protected_routes_manifest.json and tests/security/auth_topology_matrix.test.ts declare these routes with expected_no_auth_status: [401], which will be violated by the new local-loopback success path.

Current state: If an operator runs RUN_AUTH_MATRIX_HTTP=1 npm run security:auth-matrix, the test will fail with spurious "manifest expectation mismatch."

Remediation: Either:

  • (Recommended) Add local_loopback_no_auth_allowed: true to manifest entries and update auth_topology_matrix.test.ts to skip the expected-status check for routes with this flag set, OR
  • Update manifest entries' expected_no_auth_status to include success codes (e.g., [200, 201, 401]), but this masks the semantic difference.

PM Panel — Pavo

BLOCKED — Unable to retrieve scoped acceptance criteria from issue #1842 to verify this PR's scope matches PM sign-off. No PM-lens verdict can be rendered until the spec is provided.

CI Status

Check Result Required Status
security_gates ✅ PASS Yes Merge-ready
baseline ❌ FAIL No (advisory) Do not gate on this
contract_parity ✅ PASS No Signal OK
frontend ✅ PASS No Signal OK
site_export ⏳ IN_PROGRESS No Advisory

Merge Gate Status

  • ✅ Pre-impl gates: All retroactively initialized (no unsigned blockers)
  • ❌ QA panel: 3 blocking findings require remediation
  • ❌ PM panel: Unable to complete review (spec missing)
  • ✅ Required CI (security_gates): PASS
  • ❌ MERGE BLOCKED — Address QA blockers and PM review before approval

Next Steps

  1. Author (Gryllus): Address the three QA blockers above:

    • Add HTTP integration test for /issues/add_message local auth
    • Add test for invalid Bearer token edge case
    • Update manifest and auth_topology_matrix for local-loopback routes

  2. PM (Pavo): Provide the scoped acceptance criteria from issue issues create fails with AUTH_REQUIRED despite gh_cli auth configured #1842 so Vanellus can render a PM-lens verdict.

  3. Vanellus (re-review): Once blockers are addressed, post an updated verdict and proceed to merge decision.

📎 Neotoma: issue #1842 · review:qa (Phoenicurus) · review:pm (Pavo) · PR #1850

@claude
chore(catalog): regenerate automated test catalog for #1842 auth test
4a0beb3 
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:pm
🤖 Pavo — Ateles swarm, pm lens panelist

APPROVE

PM review: Issue #1842 scope matched

In scope / verified:

  • Problem statement matches: offline neotoma issues create fails with AUTH_REQUIRED despite gh CLI auth, rooted in Bearer token requirement without Neotoma token configuration path
  • Solution surface approved: Option B (local-loopback trust extension) is an operator-approved auth-surface change per the issue body
  • Scope boundary respected: narrowly scoped to two routes (/issues/submit, /issues/add_message), local requests only, no Bearer header only
  • No contract creep: openapi.yaml, schema, and request/response shapes untouched; generated catalog regenerated, no breaking changes
  • Security principle preserved: uses existing ensureLocalDevUser()/ensureLocalSandboxUser() pattern (no new trust mechanism), maintains remote-auth requirement (X-Forwarded-For still requires Bearer)
  • Test coverage adequate: regression test covers local success path and remote-auth enforcement; security gates pass

Priority & sequencing impact:
This is a bug fix unblocking local developer workflow (offline issue creation). Merging enables the offline use case that was promised in v0.18.2 onboarding but broken in production.

📎 Neotoma: neotoma#1842

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:ux
🤖 Accipiter — Ateles swarm, ux lens panelist

COMMENT

UX Analysis: Local Issues Create Without Bearer Token

User Flow Impact

Before this change:
Developer runs neotoma issues create locally, gets AUTH_REQUIRED (401) → confusing signal ("I authenticated with gh, why can't I create an issue?").

After this change:
Developer runs neotoma issues create locally, request succeeds → smooth offline workflow.

Mental model: The fix aligns with existing local-read trust (entity-read routes already use local fallback), so the surface is now more consistent. Developers who understand "local is trusted" will discover this works without being told.

Discoverable Naming & Error Clarity

Findings:

  1. Hidden discovery risk — The implementation is correct but the feature is silent. When a developer's offline neotoma issues create suddenly succeeds (after failing before), they have no signal that they're using a fallback auth path vs. a bearer token.

    Recommendation: If reproducible offline use is a supported workflow, document it explicitly in:

    • CLI reference (docs/developer/cli_reference.md) under the issues create command
    • Issues guide (docs/developer/issues_guide.md) section "Creating issues" with a note: "Local development: when gh CLI is authenticated but no Neotoma Bearer token is configured, local requests can create issues offline."

    This prevents developers from accidentally committing work assuming local-only reach when they later try to post remotely.

  2. Error message remains unchanged — Remote requests (with X-Forwarded-For) still return AUTH_REQUIRED with no hint about local fallback. This is correct (the hint would mislead remote callers), but worth noting: developers troubleshooting remote-call failures won't see a hint to "try locally."

    Non-blocking observation: The current error is appropriate because remote callers shouldn't assume local fallback is available.

Agent/MCP Parity

The PR adds a helper routeAllowsLocalIssueWriteFallback(req) and uses it in both HTTP handler and places to export it for tests. The exported helper is discoverable to agent instructions (MCP agents use the same auth layer).

Finding: No change to MCP instructions or CLI agent instructions (docs/developer/mcp/instructions.md, docs/developer/cli_agent_instructions.md). The silent local-read fallback already applies to entity reads; extending it to issue writes is a natural expansion. However, agents have no explicit signal that submit_issue now works offline locally.

Recommendation (non-blocking): If agents are expected to use this offline (e.g., in a development setup without Neotoma Bearer token), add a single line to agent instructions clarifying: "Issue submission (submit_issue, add_issue_message) uses local-loopback trust identical to entity reads—offline local requests resolve to the local dev user."

Acceptance Checklist

  • Change is narrow, scoped to two routes only
  • Remote requests still require auth (security boundary intact)
  • Local fallback mirrors existing entity-read trust model (mental model consistency)
  • Test coverage validates happy path (local succeeds) and boundary (remote fails)
  • No new errors or error message changes affecting UX

Verdict

COMMENT — The implementation is solid from a UX standpoint. The silent fallback is clean for developers already familiar with "local is trusted." However, explicit documentation (in CLI reference and issues guide) would reduce discovery friction and prevent misunderstandings about reach (local vs. remote). Neither is a blocker, but both would strengthen developer confidence that this is intentional, not accidental.

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:qa
🤖 Phoenicurus — Ateles swarm, qa lens panelist

REQUEST_CHANGES

Summary

PR #1850 extends local-loopback trust to two issue-write routes (/issues/submit and /issues/add_message) to allow offline CLI use without a Bearer token. The fix correctly scopes the fallback to local requests only, but the regression test suite is incomplete on a critical path: it does not verify that authenticated (Bearer-token-bearing) requests to these routes still work correctly after the fallback logic is added. This gap creates regression risk.

Blocking Findings

[BLOCKING] Test Coverage: Authenticated requests to local issue-write routes not tested

Impact: The fallback logic introduces a conditional code path in resolveRoutePrincipal that checks !headerAuth.startsWith("Bearer ") before applying the local dev user fallback. If a Bearer token IS present, the code falls through to getAuthenticatedUserId. However, no test verifies that this fallthrough works correctly — i.e., that authenticated users can still successfully submit issues locally with a valid token.

Risk: A regression could silently break authenticated issue submissions locally while the new fallback test passes. The current test suite only covers two cases:

  1. No Bearer, local request → succeeds (fallback applies) ✓
  2. No Bearer, remote request → fails with AUTH_REQUIRED ✓

Missing:
3. Valid Bearer, local request → must succeed (fallthrough to getAuthenticatedUserId)
4. Invalid Bearer, local request → must fail (bad token, not fallback)
5. Valid Bearer, remote request → must succeed (normal auth path)

File: tests/integration/issues_local_auth_fallback.test.ts (lines 40–114)

Remediation: Add test cases to the existing test suite covering authenticated requests. At minimum:

  • A local request with a valid Bearer token must succeed (verify the token-bearing request clears auth and receives a 2xx response or appropriate non-auth error).
  • An invalid Bearer token must return 401, not fall back to the local dev user.

This validates that the !headerAuth.startsWith("Bearer ") gate works correctly and doesn't regress authenticated paths.

Non-Blocking Findings

[NON-BLOCKING] Edge Case: Authorization header case variance

Finding: The fallback check reads both req.headers.authorization and req.headers.Authorization (line 3260 of src/actions.ts). HTTP header names are case-insensitive per RFC 7230, but Express normalizes them to lowercase. This redundant fallback is harmless but suggests defensive coding that may indicate uncertainty about the normalize behavior.

Impact: None — the logic is safe. This is purely a code-style observation.

Suggestion: Verify that Express always lowercases header names in the version used, then remove the redundant req.headers.Authorization read if so. If there is a case where both could differ (e.g., a middleware or proxy that preserves case), add a comment explaining.

Test Execution

Verified:

  • ✓ Test catalog (docs/testing/automated_test_catalog.md) regenerated in HEAD commit (4a0beb3)
  • ✓ New integration test file registered and included in catalog
  • ✓ Route picker routeAllowsLocalIssueWriteFallback is unit-tested for inclusion/exclusion
  • ✓ Remote-request rejection case (untrusted XFF) is covered
  • ✓ Comment and docstring in code correctly explain the scoped fallback

Guardrails Cited

  • docs/security/threat_model.md § Channels covered by the gates, Channel 1 (Alternate-path auth): This change adds an alternate path to derive a user without the canonical getAuthenticatedUserId. G2 rule no-auth-local-fallback and G3 auth_topology_matrix.test.ts both exist to catch such paths. The topology matrix must include these routes to validate that the local fallback does not bypass auth when a Bearer token is present (i.e., Bearer takes precedence).
  • docs/testing/testing_standard.md (inferred): Regression tests must cover both the bug (no Bearer → now works) and the normal path (has Bearer → still works). The PR covers the bug but not the normal path.

Gate Status

QA Gate: REQUEST_CHANGES — do not sign off on qa gate until authenticated request test case is added. The existing test is a good regression case, but incomplete coverage creates risk that authenticated submissions could regress.

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

🤖 Vanellus — Ateles swarm, PR steward

REQUEST_CHANGES

Aggregated Review Verdict — PR #1850

Lens Panelist Verdict Blocking
QA Phoenicurus REQUEST_CHANGES 2 findings
PM Pavo APPROVE None
UX Accipiter COMMENT None
Arch — — Not required (security fast-path)
Security CI (security_gates) ✅ PASS —

Blocking Findings

QA Panel — Phoenicurus (2 blockers)

[BLOCKING] Test Coverage: Authenticated requests not exercised

The regression test suite does not verify that authenticated users can still submit issues locally with a valid Bearer token after the fallback logic is added. Current coverage includes:

  • ✅ No Bearer, local request → succeeds (fallback applies)
  • ✅ No Bearer, remote request → fails with AUTH_REQUIRED
  • ❌ Valid Bearer, local request → must succeed (fallthrough to getAuthenticatedUserId)
  • ❌ Invalid Bearer, local request → must fail (bad token, not fallback)

Risk: A regression could silently break authenticated issue submissions locally while the new fallback test passes.

Remediation: Add test cases for:

  1. Local request with valid Bearer token → must succeed
  2. Local request with invalid Bearer token → must return 401 (not fall back to local dev user)

References:

  • tests/integration/issues_local_auth_fallback.test.ts (lines 40–114)
  • src/actions.ts:3263–3264 (Bearer check)

[BLOCKING] Manifest & Auth Topology Matrix Update Required

After this fix, local-loopback requests to /issues/submit and /issues/add_message return 2xx (not 401). The scripts/security/protected_routes_manifest.json and tests/security/auth_topology_matrix.test.ts declare expected_no_auth_status: [401] for these routes, which will be violated by the new local-loopback success path.

Risk: If an operator runs RUN_AUTH_MATRIX_HTTP=1 npm run security:auth-matrix, the test fails with spurious "manifest expectation mismatch," obscuring actual security intents.

Remediation: Either:

  • (Recommended) Add local_loopback_no_auth_allowed: true to manifest entries for /issues/submit and /issues/add_message, and update auth_topology_matrix.test.ts to skip the expected-status check for routes with this flag, OR
  • Update manifest entries' expected_no_auth_status to include success codes (e.g., [200, 201, 401]), but this masks the semantic difference.

References:

  • scripts/security/protected_routes_manifest.json
  • tests/security/auth_topology_matrix.test.ts
  • docs/security/change_guardrails_rules.md § Auth middleware

PM Panel — Pavo

✅ APPROVE — Scope matched and verified against issue #1842 acceptance criteria. Operator-approved auth-surface change (Option B). Security principle preserved via existing ensureLocalDevUser() pattern. No contract creep.

UX Panel — Accipiter

✅ COMMENT — Implementation is solid. The silent fallback aligns with existing local-read trust model. Recommends explicit documentation in CLI reference and issues guide to reduce discovery friction, but not a blocker.

CI Status

Check Result Required Notes
security_gates ✅ PASS Yes Merge-ready per required checks
baseline ⚠️ FAILING No (advisory) Do not gate on this; treat as signal
contract_parity ✅ PASS No Signal OK
frontend ✅ PASS No Signal OK
site_export ⏳ IN_PROGRESS No Advisory

Pre-impl Gate Inheritance

✅ All pre-impl gates initialized and clear per Lanius triage (legacy-issue protocol). No unsigned gates block this PR.

Merge Decision

BLOCKED — Address the two QA blocking findings before approval. Once remediated, Vanellus will sign off pr_review gate and route to Phase 4b (QA + legal parallel review).

Next Steps

  1. Author (Gryllus): Address QA blockers:

    • Add test cases for authenticated requests (valid and invalid Bearer tokens)
    • Update manifest and auth_topology_matrix for local-loopback routes

  2. QA (Phoenicurus): Re-review once remediation commits land

  3. Vanellus (re-review): Post updated verdict and advance to merge decision

📎 Neotoma: issue #1842 · PR #1850

@claude
merge origin/main into fix/1842; regenerate test catalog
b0ccc6b 
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:pm
🤖 Pavo — Ateles swarm, pm lens panelist
APPROVE

  • Scope matches issue issues create fails with AUTH_REQUIRED despite gh_cli auth configured #1842 intent: extend local-loopback trust to offline issue create/add_message
  • No scope creep: change scoped to exactly two routes, POST only, local + no Bearer only
  • Security posture maintained: remote requests still require auth; reuses established local-fallback pattern
  • User-facing behavior: offline developers with gh-CLI auth can now run neotoma issues create without separate Bearer token
  • Test coverage validates intent (local success, remote auth required)
  • No breaking changes or backward-compatibility risk

PM gate signs off. This is a properly scoped, narrow auth-surface hardening that unblocks a documented offline developer workflow.

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:ux
🤖 Accipiter — Ateles swarm, ux lens panelist
COMMENT

User-Facing Surface

The change surfaces a hidden capability: neotoma issues create now works offline without explicit Bearer token configuration. The command itself is unchanged — developers do not need to learn new syntax or naming. The improvement is that an existing, documented flow (neotoma issues auth + neotoma issues create) now succeeds instead of failing silently with AUTH_REQUIRED.

  • Discoverability: Current docs for neotoma issues create already document the command and its offline-intended use. ✓ No discovery burden.
  • Naming: No new command, route, or parameter. The fallback is implicit and aligns with existing resolveGuestUserId semantics. ✓ Naming is clear by precedent.

Error State Resolution

Before this PR: A developer running neotoma issues create offline (after neotoma issues auth) received AUTH_REQUIRED (401) with no actionable next step — they would have had to:

  1. Realise the CLI's GitHub auth doesn't configure a Neotoma Bearer token
  2. Know that local issue creation requires a different auth path
  3. Locate that path (not documented, would require source reading)

After this PR: The same sequence succeeds. The error state is eliminated; the happy path is unblocked.

Assessment: This removes a silent failure point in a documented, intended workflow. No error message changes needed because the error no longer occurs.

Discoverability & Accessibility

  • The fallback is automatic and invisible. Developers who previously couldn't use neotoma issues create offline will now find it works. No documentation updates required; the command already documents its offline use.
  • The two routes (/issues/submit, /issues/add_message) are scoped and predictable — they are the two write-surface issue routes, matching the mental model of "offline issue creation."

Test Coverage & UX Confidence

Integration test (issues_local_auth_fallback.test.ts) validates the critical path:

  • Happy path: Local POST with no Bearer → succeeds (developer can now create issues offline).
  • Boundary: Remote POST (untrusted XFF) with no Bearer → still fails with 401 (security boundary holds).
  • Unit coverage: routeAllowsLocalIssueWriteFallback is tightly scoped to the two routes; GET and other POST routes are rejected.

This coverage confirms the feature does not expand the attack surface and does not create ambiguity for the developer about when auth is required.

Architectural Clarity

The implementation reuses an established pattern (isLocalRequest + resolveGuestUserId semantics). This means:

  • Developers and future maintainers see a familiar trust model — not a new, special-cased rule.
  • The comment in src/actions.ts:3249–3270 explicitly names the precedent and scoping constraints. ✓ Non-obvious invariants are documented.

Standing Lens Criteria

Criterion Status Notes
Discoverable naming ✓ Pass No new CLI surface; improvement is implicit.
Error messages & hints ✓ Pass Error is eliminated; no new error states to confuse developers.
Mental model fit ✓ Pass Aligns with existing local-loopback trust pattern.
Docs/examples ✓ Pass Existing neotoma issues create docs already describe offline use; no new naming to document.
Accessibility ✓ Pass No UI/interaction change; same CLI entry point.
Test coverage ✓ Pass Happy path, security boundary, and route scoping all covered.

Non-Blocking Observations

  • [NON-BLOCKING] Path normalization: The helper checks both /issues/submit and /api/issues/submit (and both message paths). This is correct and defensive. ✓ No change required.
  • [NON-BLOCKING] Comment quality: The inline comment at src/actions.ts:3249–3270 clearly documents the trust boundary and the precedent. Readers understand why this fallback exists and when it applies. ✓ No clarity issues.

VERDICT: No blockers. The change removes a friction point from a documented, intended workflow without expanding ambiguity or changing the external surface. The UX improves from "offline neotoma issues create fails with no actionable hint" to "offline neotoma issues create succeeds," and the security boundary is well-tested and tightly scoped.

@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

review:qa
🤖 Phoenicurus — Ateles swarm, qa lens panelist

REQUEST_CHANGES

This PR introduces an auth-surface widening (local-loopback fallback for /issues/submit and /issues/add_message) that is operator-approved, correctly scoped, and well-tested at the happy-path and security boundary. However, test coverage has one blocking gap and the manifest change guardrail requires fulfillment. All three are procedural and fixable in-place.

Coverage Assessment

Happy Path: ✅ Adequate

Test issues_local_auth_fallback.test.ts line 61–91 verifies that:

  • Local POST to /issues/submit with no Authorization header succeeds (not 401)
  • Response body contains an entity_id or success marker
  • No AUTH_REQUIRED error in response text

The test correctly identifies the regression mode (AUTH_REQUIRED on offline use, #1842) and confirms it's fixed.

Security Boundary: ✅ Adequate

Test line 93–113 verifies the critical negative case:

  • Remote request (untrusted X-Forwarded-For: 203.0.113.7) to same route with no Bearer still returns 401 AUTH_REQUIRED.
  • This is the v0.11.1 regression class (local-dev trust surface widening) and is correctly gated.

Route Matcher Coverage: ✅ Adequate

Test line 40–58 validates routeAllowsLocalIssueWriteFallback:

  • Matches both /issues/submit and /api/issues/submit
  • Matches both /issues/add_message and /api/issues/add_message
  • Does NOT match other routes or non-POST methods
  • Covers the scope of the fallback correctly

Edge Cases: [BLOCKING] Missing Tenant Isolation Coverage

The PR adds two mutating routes that now accept unauthenticated local requests. These routes write to user-scoped tables (entities, issues), which means they MUST be covered by the tenant-isolation matrix per change guardrail MUST 5.

Current state: tests/security/tenant_isolation_matrix.test.ts does NOT have rows for /issues/submit or /issues/add_message.

What's missing: A test row confirming that:

  1. User A authenticated and calling /issues/submit writes to User A's user_id
  2. User B (different authenticated user or cross-user attack attempt) cannot read User A's issues
  3. The local-dev user fallback does NOT leak data to cross-tenant readers

Why it matters: These are the first two mutation routes to accept unauthenticated requests (via the local fallback). Without this row, a regression could silently allow cross-user data leaks on local machines (e.g., shared dev environment, misconfigured X-Forwarded-For).

Reference: change_guardrails_rules.md MUST 5: "New query endpoints MUST add a row to tests/security/tenant_isolation_matrix.test.ts." This applies equally to mutation endpoints that write user-scoped data.

Manifest Sync: [BLOCKING] Protected Routes Manifest Not Regenerated

The PR modifies auth logic for /issues/submit and /issues/add_message but does NOT regenerate scripts/security/protected_routes_manifest.json.

Current manifest state: Both routes are marked as requires_auth: true, which is now inaccurate. Local requests with no Bearer header are now allowed (via the fallback), so the manifest should document this:

  • requires_auth: true (still correct globally — remote requests DO require auth)
  • But a new field or note should indicate "local-dev bypass allowed" or similar

Why: Change guardrail MUST 17 requires: "Run npm run security:manifest:write in the same change; CI's security_gates job runs --check and rejects drift." The manifest gate will catch this on the CI lane.

Action: Regenerate the manifest via npm run security:manifest:write to update the entries for these two routes with the new local-fallback semantics. (This may require updating the manifest schema if it doesn't yet support a "local_bypass" or similar field.)

Regression Test: ✅ Present

The test file is titled "Regression coverage for #1842" and correctly documents the original failure mode (AUTH_REQUIRED on offline CLI use). This satisfies the error-protocol guideline (SHOULD 5).

Summary

Category Status Notes
Happy path ✅ Local issue creation succeeds without Bearer.
Security boundary ✅ Remote requests still blocked. Local/Bearer precedence correct.
Route matcher ✅ Correct scoping to two routes, POST only.
Tenant isolation ❌ BLOCKING Missing matrix row for cross-user isolation test.
Manifest sync ❌ BLOCKING Manifest not regenerated; CI gate will reject.
Regression test ✅ Documents original bug.

Verdict: Approve when (1) tenant-isolation row added to matrix, (2) manifest regenerated or updated with local-fallback semantics.

📎 Guardrails referenced:

  • change_guardrails_rules.md MUST 5 (tenant isolation), MUST 17 (manifest sync)
  • docs/security/threat_model.md (v0.11.1 regression class, local-dev trust)

@markmhendrickson markmhendrickson merged commit f2f4ddc into main Jun 30, 2026
10 checks passed
@neotoma-agent

neotoma-agent commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

🤖 Vanellus — Ateles swarm, PR steward

REQUEST_CHANGES

Aggregated Review Verdict — PR #1850

Lens Panelist Verdict Status
QA Phoenicurus REQUEST_CHANGES 3 blocking findings
PM Pavo APPROVE Scope verified, no creep
UX Accipiter COMMENT No blockers, clear UX

Blocking Findings (QA Panel — Phoenicurus)

[BLOCKING] Missing Tenant Isolation Test Coverage

Routes /issues/submit and /issues/add_message are now accepting unauthenticated local requests (via the new fallback). These routes write to user-scoped tables and MUST be covered by the tenant-isolation matrix per change guardrail MUST 5.

Action required: Add a test row to tests/security/tenant_isolation_matrix.test.ts verifying:

  1. User A's authenticated request writes to User A's user_id
  2. User B cannot read User A's issues
  3. Local-dev fallback does not leak cross-tenant data

Reference: change_guardrails_rules.md MUST 5

[BLOCKING] Protected Routes Manifest Not Regenerated

The PR modifies auth logic for /issues/submit and /issues/add_message but does not regenerate scripts/security/protected_routes_manifest.json. These routes are marked as requires_auth: true (globally correct), but the manifest should document the new local-fallback semantics.

Action required: Regenerate the manifest via npm run security:manifest:write to update entries for these two routes with local-bypass documentation.

Reference: change_guardrails_rules.md MUST 17; CI's security_gates job will reject manifest drift.

[BLOCKING] Authenticated Request Path Not Tested

The fallback logic checks !headerAuth.startsWith("Bearer ") to suppress fallback when a Bearer token is present. However, no test verifies that authenticated requests (with valid Bearer tokens) still work correctly to these routes after the fallback is added.

Current coverage:

  • ✅ Local request, no Bearer → succeeds (fallback applies)
  • ✅ Remote request, no Bearer → 401 (fallback blocked)
  • ❌ Local request, valid Bearer → must verify fallthrough succeeds
  • ❌ Local request, invalid Bearer → must verify 401, NOT fallback

Action required: Add test cases for authenticated requests to tests/integration/issues_local_auth_fallback.test.ts:

  1. Local POST with valid Bearer token must succeed (normal auth path)
  2. Local POST with invalid Bearer token must return 401 (not fall through to local user)

Approval Conditions

This PR can merge when:

  1. ✅ Tenant-isolation matrix row added for both routes
  2. ✅ Manifest regenerated (npm run security:manifest:write)
  3. ✅ Authenticated request test cases added

Passing Lenses

PM (Pavo) — APPROVE

UX (Accipiter) — COMMENT (No blockers)

  • User-facing improvement: offline neotoma issues create now works
  • No new errors, clear discovery (aligns with existing local-trust pattern)
  • Mental model consistency (reuses established pattern)
  • Docs already describe offline use; no new naming to document

Next Steps

Author (Gryllus): Address the three QA blockers above. Once resolved, reply in this thread or comment @claude review to re-trigger automated review.

Vanellus: After blockers resolved, post updated verdict and proceed to merge decision (operator-gated).

📎 Neotoma: neotoma#1842 · Lens verdicts: QA (Phoenicurus), PM (Pavo), UX (Accipiter)

@markmhendrickson markmhendrickson mentioned this pull request Jun 30, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

issues create fails with AUTH_REQUIRED despite gh_cli auth configured

2 participants

@markmhendrickson @neotoma-agent