Repository navigation
test: make reserved ACP path assertion feature-aware - #2410
Conversation
Without the `acp` feature GET /acp returns 404 (reserved but unrouted). With the `acp` feature the route is mounted as POST-only, so GET /acp returns 405. Both statuses prove the console shell did not shadow the path — the invariant the test exists to protect — but the hard-coded NOT_FOUND assertion failed deterministically under `--features acp`. Pull /acp out of the NOT_FOUND loop and assert the feature-appropriate code via `cfg!(feature = "acp")`, keeping the "no <title>console</title>" body check unconditional in both branches. Fixes tinyhumansai#1979 Co-authored-by: Cursor <[email protected]>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reached
This review includes 3 billable files and costs up to $0.75. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe route test checks ChangesACP route tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other · Severity of issue fixed: Low Suggested reviewers: Merge Risk: ⚪ Minimal · up to The ACP route expectations are covered in both feature configurations. No issue identified here needs resolution before merge. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
A rabbit checks the paths at night, Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The test now matches router behavior in both feature states while preserving the intended invariant.
Pull request overview
Updates the reserved-path test to account for the feature-gated, POST-only /acp route.
Changes:
- Removes
/acpfrom the unconditional 404 loop. - Expects 404 without
acpand 405 withacp. - Preserves the no-SPA-shell body assertion.
File summaries
| File | Description |
|---|---|
crates/opencompany-core/src/server/routes_tests.rs |
Adds feature-aware /acp status assertions. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 1 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Incomplete Review snapshot
Completeness: Incomplete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Resolved this pass
Pending checks: Console E2E, Console E2E (live brain), Console E2E (first run) Could not review: .github/workflows/ci.yml, crates/opencompany-core/src/server/routes_tests.rs, scripts/ci/feature-lanes.txt Before merge
How this fits togetherflowchart LR
n0["..._does_not_shadow_unmatched_reserved_paths<br/>changed<br/>1 finding"]:::flagged
n1["assert"]:::impacted
n2["router_with_console"]:::impacted
n3["console_fixture"]:::impacted
n4["with_home"]:::impacted
n5["spec_body"]:::impacted
n6["spec_serves_no_absolute_path_in_any_field"]:::impacted
n0 -->|calls| n1
n0 -->|tests| n1
n0 -->|calls| n2
n0 -->|tests| n2
n0 -->|calls| n3
n0 -->|tests| n3
n6 -->|calls| n1
n6 -->|tests| n1
n6 -->|calls| n4
n6 -->|tests| n4
n6 -->|calls| n5
n6 -->|tests| n5
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0059 · 140,241 in / 6,125 out · 7,642 cached (5%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 237 embedded
critique: $0.0028 · 58,887 in / 1,213 out · 4,058 cached (7%) · gpt-5.6-luna
security: $0.0022 · 38,633 in / 476 out · 0 cached (0%) · gpt-5.6-luna
tests: $0.0003 · 15,599 in / 1,129 out · 1,024 cached (7%) · deepseek-v4-flash
description: $0.0001 · 8,063 in / 194 out · 1,024 cached (13%) · deepseek-v4-flash
e2e: $0.0003 · 16,508 in / 1,432 out · 1,536 cached (9%) · deepseek-v4-flash
tinysweeper
Bot
added
the
priority: p2
Apply the two rustfmt corrections CI reported: - Collapse the /acp Request builder into a single-line oneshot call. - Reformat the trailing assert! into the parenthesised multiline shape. Add a third run-scoped-suite invocation to the 'Test the ACP surface' step (server::routes, features acp,runner,tinymemory). Without it the 405 branch of console_does_not_shadow_unmatched_reserved_paths — which only fires when the acp feature is on — was never exercised by CI. This is the coverage gap issue tinyhumansai#1979 identified. Update scripts/ci/feature-lanes.txt to record server::routes as a filter in the acp partial lane, keeping the table accurate. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0088 · 185,456 in / 17,087 out · 6,960 cached (4%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 355 embedded
critique: $0.0048 · 87,306 in / 4,762 out · 2,066 cached (2%) · gpt-5.6-luna, deepseek-v4-flash
security: $0.0028 · 51,135 in / 2,025 out · 1,822 cached (4%) · gpt-5.6-luna
tests: $0.0003 · 16,833 in / 1,529 out · 1,024 cached (6%) · deepseek-v4-flash
description: $0.0002 · 8,953 in / 2,550 out · 1,024 cached (11%) · deepseek-v4-flash
e2e: $0.0004 · 17,723 in / 3,695 out · 1,024 cached (6%) · deepseek-v4-flash
Pin a no-acp CI invocation for the reserved-path assertion and enrich failure messages with status/body context so a regression is backtraceable. Co-authored-by: Charan Rathore <[email protected]>
tinysweeper could not review the latest push, so its earlier approval no longer speaks for this pull request.
There was a problem hiding this comment.
tinysweeper found nothing blocking, but could not review everything, so this is not an approval: tinysweeper/description, tinysweeper/e2e.
$0.0112 · 167,620 in / 8,254 out · 4,136 cached (2%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 387 embedded
critique: $0.0062 · 91,397 in / 4,058 out · 4,136 cached (5%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0042 · 55,302 in / 2,643 out · 0 cached (0%) · gpt-5.6-luna
tests: $0.0006 · 16,920 in / 106 out · 0 cached (0%) · deepseek/deepseek-v4-flash
…9-acp-reserved-path-test # Conflicts: # .github/workflows/ci.yml # scripts/ci/feature-lanes.txt
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0277 · 144,920 in / 14,034 out · 25,295 cached (17%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 389 embedded
critique: $0.0104 · 74,604 in / 1,719 out · 22,223 cached (30%) · gpt-5.6-luna
security: $0.0115 · 27,931 in / 748 out · 0 cached (0%) · gpt-5.6-luna
tests: $0.0019 · 14,379 in / 3,803 out · 1,280 cached (9%) · deepseek/deepseek-v4-flash
description: $0.0007 · 6,267 in / 1,470 out · 1,280 cached (20%) · deepseek/deepseek-v4-flash
e2e: $0.0021 · 15,267 in / 3,836 out · 0 cached (0%) · deepseek/deepseek-v4-flash
tinysweeper could not review the latest push, so its earlier approval no longer speaks for this pull request.
There was a problem hiding this comment.
tinysweeper found nothing blocking, but could not review everything, so this is not an approval: .github/workflows/ci.yml, crates/opencompany-core/src/server/routes_tests.rs, scripts/ci/feature-lanes.txt.
$0.0046 · 30,530 in / 12,881 out · 7,168 cached (23%) · deepseek/deepseek-v4-flash
tests: $0.0015 · 13,045 in / 4,062 out · 5,888 cached (45%) · deepseek/deepseek-v4-flash
description: $0.0008 · 6,326 in / 1,724 out · 1,280 cached (20%) · deepseek/deepseek-v4-flash
e2e: $0.0014 · 6,943 in / 4,130 out · 0 cached (0%) · deepseek/deepseek-v4-flash
|
Thanks for the review. On the missing end-to-end test: the assertion in Both feature arms run in CI after this PR (
What this does not do is bind a TCP listener and make a network request from a separate client. If you want that as well, I can add it as a follow-up, but I would rather not push an untested Rust test. I could not compile or run the tests in my environment, so I have not run them since this review. |
Summary
Fixes #1979.
console_does_not_shadow_unmatched_reserved_pathshard-codedNOT_FOUND(404) for every path in its loop, including
/acp. That claim is only truein a build without the
acpfeature. Whenacpis on, the route ismounted as POST-only (
Router::new().route("/acp", post(…))inserver/acp/transport.rs), so a GET receives405 Method Not Allowedfrom the Axum dispatcher before the reserved-path fallback is consulted.
Nothing catches this in CI:
feature-lanes.txtlistedacpaspartial,and the lane's
cargo testfilter (server::acp harness::acp::run_turn)never selected
server::routes. It surfaced only on an unscoped run — exactlythe issue #770 shape the contributor guide describes.
Root cause
The test expressed an implementation detail (specific 4xx code) rather than
the invariant (reserved paths are never answered by the SPA shell). The
invariant holds for both 404 (no route mounted) and 405 (route mounted but
wrong method); the test was only checking one of the two states.
What changed
Test fix — pull
/acpout of the sharedNOT_FOUNDloop and add adedicated block that selects the expected status code at compile time via
cfg!(feature = "acp"):acp404 Not Foundacp405 Method Not AllowedThe body assertion —
!body.contains("<title>console</title>")— isunconditional in both branches. That is the invariant: neither code may ever
be the SPA shell.
CI — adds a third
run-scoped-suite.shinvocation forserver::routesunder the
acp,runner,tinymemoryfeature set. Without this step, the405branch was never exercised by CI; the default lane (cargo test --locked) covers the404branch (no-acp) but not the405one.feature-lanes.txt—acprow updated to includeserver::routesinits filter list, reflecting the new CI coverage.
Why this approach
Test the property (no HTML shell), not the specific code, using
cfg!toencode the per-feature expectation inline. The comment above the
cfg!blockexplains both cases, making the feature matrix visible to the next reader.
The remaining paths in the loop (
/api/v1/does-not-exist,/.well-known/…)are unaffected; those routes are genuinely absent in all configurations and
correctly return 404.
Behavior / API impact
No behavior change. This is a test and CI correctness fix only.
Tests
cargo test --locked --features acp,runner,tinymemory --lib server::routesnowpasses (it previously failed with
left: 405, right: 404on the/acpassertion).cargo test --locked --lib server::routescontinues to pass unchanged.Commands run locally
Summary by CodeRabbit
/acppath now returns the appropriate response depending on whether ACP is enabled.