fix: resolve all 102 SonarQube issues + boost test coverage - #5
Merged
Conversation
Issues fixed: - S1128: Remove 14 unused imports across 4 hook files - S3863: Merge 10 duplicate imports across 5 hook files - S7741: Add NOSONAR to 12 SSR typeof guards (intentional pattern) - S6759: Add Readonly<> to 14 component prop types across 6 files - S4325: Remove unnecessary type assertions - S7735: Swap 4 negated ternary conditions - S1854: Remove 2 useless assignments - S6754: Fix 1 useState destructuring - S6606: Use ??= operator - S1082: Add keyboard listeners to 2 clickable elements - S7721: Move 3 functions to outer scope - S3358: Flatten 5 nested ternaries - S2004: Extract 2 deeply nested functions - S107: Refactor buildArticleClassName to use options object - S6479: Fix array index key in login - S6478: Extract component from parent in breakdown-suggestion - S6847/S6845/S6848: Fix 4 accessibility issues - S3776: Reduce cognitive complexity in upgrade (37->15), login (33->15), habit-request-builders (21->15) Coverage improvements: - 86 new tests for shared package Zod schemas (9 type files) - 40 new tests for profile sub-components - 33 new tests for habit-form-fields - 12 new tests for error pages - 16 new tests for use-habits mutations - Additional tests for goal-list, push-prompt, trial-expired-modal Total: 1339 tests across 120 files, all passing. 0 type errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
thomasluizon
added a commit
that referenced
this pull request
Apr 15, 2026
…ferral/JWT
Security hardening on the web BFF auth layer:
- Dedicated /api/auth/{send-code,verify-code,google,logout} routes now call
buildForwardedClientHeaders(request) so X-Forwarded-For / CF-Connecting-IP /
X-Orbit-Country-Code / Accept-Language / X-Orbit-Time-Zone reach the .NET
backend. Previously these routes sent no client context, so the backend's
DistributedRateLimitFilter partitioned every anonymous auth request under
the Next.js server IP — collapsing the per-client 5 req/min bucket.
- auth_token cookie maxAge changed from 86_400 (1 day) to 604_800 (7 days) so
it matches the backend JwtSettings.ExpiryHours=168. Prevents daily forced
token refresh round-trips.
- Length-cap the JWT payload segment in /api/auth/session before JSON.parse
(4 KB ceiling). Blocks a memory-amplification vector where a malicious
cookie contains a megabyte-scale base64 segment.
- Cap referral query-param at 40 chars before cookie write on the login page.
Includes new unit tests for send-code header forwarding and updates
auth-api.test.ts to assert the 7-day maxAge.
Covers frontend plan Area A #1, #2, #4, #5.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thomasluizon
added a commit
that referenced
this pull request
Apr 15, 2026
Non-shippable fixes from the audit that require architectural decisions, new migrations, or large refactors have TODO comments citing PLAN.md: - auth-api.ts: tryRefreshSession mutex — needs single-flight dedup (Area A #6) - actions/chat.ts: server-persisted conversations — needs DB migration + API contract change (coordinated with backend F4 / P0 #5) - habit-list.tsx: split into smaller components (Area C #1) - habit-list.tsx: mobile parity — port bulk-action-bar, controls-menu, today-filters from web (Area E #1) Each TODO references PLAN.md with the audit section + priority so the follow-up PR author can find the context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced May 18, 2026
40 tasks
This was referenced Jun 27, 2026
Merged
thomasluizon
added a commit
that referenced
this pull request
Jul 23, 2026
…e call Second review round on this PR found bypass #6: the fix for #5 only recognised a member chained straight onto require(), so splitting it across two statements put the write behind an allowlisted call-site name again. node -e "const m=require('fs'); const map=m.writeFileSync; map(signoff,'{}')" Reproduced before changing anything: three of four write shapes were ALLOWED, and the reviewer traced it by hand because its sandbox could not execute node. The one shape already blocked was blocked for an unrelated reason. The classifier now tracks the module REFERENCE rather than the syntax around it. An identifier bound to a require() of a module is followed through plain rebinding, so a chain of aliases inherits it, and every member lifted off any of them must be a read API. A computed member is refused because the name is not visible. A required .json stays DATA, so reading .cells off the manifest still works. Renaming even a read is still refused by the callee scan: this list grants nothing, it only withholds. Replaced the per-identifier dynamic RegExp with static patterns after the first attempt built its source in a template literal, where \b and \s are JS escapes, not regex ones - it threw at parse time and every command sailed through while it did. Static patterns cannot fail that way. Five regression tests cover the two-step, transitive-rebind, computed-member and destructure-off-a-bound-ref forms, plus the honest read that must keep working. 745 assertions, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt
thomasluizon
added a commit
that referenced
this pull request
Jul 23, 2026
* chore(harness): bring the /drive work-order harness to main The harness that hands an unattended agent a scoped work order and then measures whether it actually did the work has lived only on feature/539-b5-apply-design. Nothing on main could run it, so /drive was usable for exactly one issue. This ports the tooling itself. What lands: - tools/workorder.mjs, drive-queue.mjs, check-diff-ownership.mjs, surface-manifest.mjs, check-surface-coverage.mjs, visual-signature.mjs and the visual-capture helpers - .claude/skills/drive/run.mjs, the engine SKILL.md references but main lacked - forbid-gate-tamper + surface-coverage-gate hooks and their shared rule core - the context reorganisation: .claude/rules/core.md stays always-loaded, activity-scoped judgement moves to .claude/playbooks/ - a surfaces manifest and 169 work orders derived from MAIN's own router and component graph, not the design branch's Five independent review rounds hardened this before it moved. The load-bearing outcomes: the ownership gate judges an explicit base...head range and refuses to guess one, so a child that commits an escape and checks out the base branch no longer scores clean; an empty range is a failure, not a pass, which is what stops a child that produced nothing from being recorded ready-for-review; and debt clearance is labelled a lint-count axis with the depth measurement printed beside it, because clearing lint is not evidence that a pixel moved. Deliberately NOT included, because they are #539 design work rather than harness: the spacing-scale and no-arbitrary-zindex lint rules, the eslint config changes that flip no-decorative-glow and no-raw-gradient to error (main still has the glow those rules ban), and the populated suppressions ledgers. The ledgers land empty so the tools have their required input; they fill when the rules arrive with the design work. Two fixes main needed to accept the port: - the typed-uppercase corpus guard becomes a ratchet instead of asserting zero. main carries two pre-existing values per locale, and one of them, confirmPlaceholder "ORBIT", is load-bearing: the user types it verbatim to confirm account deletion, so it must never be recased. - tools/visual-signature.mjs carried a stray NUL byte that made git treat a source file as binary and its diff unreviewable. 733 hook assertions pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt * fix(harness): close the alias bypass in the tamper guard and finish the port's deletions Addresses the three findings from the review of this PR. Critical - the signoff tamper guard allowlisted a call by the identifier at the call site, never by what that identifier was bound to. Since the allowlist has to carry generic names for their legitimate uses (map, filter, parse, test), `const map = require("fs").writeFileSync; map(signoff, "fake")` walked straight through. I reproduced all four shapes the reviewer traced by hand, plus a destructured rename and a computed member, before changing anything: four of five were ALLOWED. Now the member lifted off a MODULE must itself be a read API, a computed member is refused because the name is not visible, and a destructured binding is judged by the keys it lifts rather than the names it gives them. A required .json stays DATA, so `require("./surfaces.json").cells` still reads - that is the pattern this hook's own message advertises, and over-blocking it is what tempts a future session to disarm the hook. Renaming even a read API is refused, fail-closed: the guard judges names, and the Read tool covers the rest. Six regression tests pin every shape; the suite is 740 assertions, 0 failures. Medium - the reviewer found flag-new-todos.mjs left behind with an import of checkNewTodos, which this port removes. The cause was broader than the one file: `git checkout <ref> -- <dir>` adds and updates but never deletes, so every path the harness branch had removed survived the port. That was 18 files, not one: the dead hook, the rules README and the three rule files that became playbooks, the /execute and /night-run skills that /drive replaces, and the eight .opencode/agents mirrors whose deletion is a locked decision the ported CLAUDE.md already documents. eslint-rules/no-draggable-onscroll.cjs is deliberately KEPT - it is main's own rule from #568, absent from the branch only because the branch predates it. Low - removed the stale comment claiming the locale corpus carries zero typed-uppercase violations, which contradicted the ratchet directly below it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt * fix(harness): track the module reference, not the shape of the require call Second review round on this PR found bypass #6: the fix for #5 only recognised a member chained straight onto require(), so splitting it across two statements put the write behind an allowlisted call-site name again. node -e "const m=require('fs'); const map=m.writeFileSync; map(signoff,'{}')" Reproduced before changing anything: three of four write shapes were ALLOWED, and the reviewer traced it by hand because its sandbox could not execute node. The one shape already blocked was blocked for an unrelated reason. The classifier now tracks the module REFERENCE rather than the syntax around it. An identifier bound to a require() of a module is followed through plain rebinding, so a chain of aliases inherits it, and every member lifted off any of them must be a read API. A computed member is refused because the name is not visible. A required .json stays DATA, so reading .cells off the manifest still works. Renaming even a read is still refused by the callee scan: this list grants nothing, it only withholds. Replaced the per-identifier dynamic RegExp with static patterns after the first attempt built its source in a template literal, where \b and \s are JS escapes, not regex ones - it threw at parse time and every command sailed through while it did. Static patterns cannot fail that way. Five regression tests cover the two-step, transitive-rebind, computed-member and destructure-off-a-bound-ref forms, plus the honest read that must keep working. 745 assertions, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt * test(harness): pin the rest of the tamper-bypass class The reference tracking that closed bypass #6 should make the neighbouring shapes fall out for free. Probed rather than waited for: write streams, deletions, the promises submodule off fs, fs/promises, child_process, and a path built by concatenation are all refused. Pinned so a later change to the classifier cannot quietly reopen one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt * fix(harness): judge the path by the value it builds, and fail the fence closed Third review round on this PR found bypass #7, and it was right about my own test too. Every path rule matches a CONTIGUOUS literal in the raw command text, so splitting the path across concatenated string literals means no rule fires and the segment is never examined at all. The test I added last round fragmented only the FILENAME, so the directory rule still matched an untouched token - it passed for the wrong reason and left the hole open. Verified before fixing: three of four fragmentation shapes were ALLOWED, including writes to both signoff.json and surfaces.json. Adjacent same-quoted literals are now joined before the path rules run, so the path is judged by the value it builds rather than the way it is spelled. Separately, and worse: the adapter caught every internal error and exited 0. A fence that opens when its own code throws is not a fence. This was not theory - my first attempt at the #6 fix built a regex inside a template literal, where \b and \s are string escapes rather than regex ones. It threw on every call, and every payload sailed through while it did. The catch now exits 2 with an explanation, so a command the guard cannot classify is refused rather than waved through. Proven by stubbing the rule core to throw. Also carried forward from the review: WORKFLOW.md named the deleted /execute skill and still described bundles as opening draft PRs, which contradicts the ready-for-review canon; and the flag-new-todos removal left an orphaned fixture whose MARK constant no longer existed. Twenty write shapes across four adversarial batteries are refused; five honest reads still pass, including require('./x.json').cells and a jq read. 755 assertions, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt * fix(harness): stop matching obfuscated paths; refuse the interpreter instead Fourth review round found bypass #8: joinConcatenatedLiterals needed the quote character to match on both sides of the +, so a mixed-quote split walked past it. Verified, and it is worse than reported - two more shapes I went looking for also walked past: splitting the path across variables, and assembling it with Array.join. The suggested fix (drop the backreference) closes only the first. That is the whole lesson of the last four rounds. Each one closed a different spelling of "hide the path" - an alias, a split literal, a mixed quote pair, a variable - and the next round found the next spelling. Deciding what a path argument evaluates to inside arbitrary code is not something a regex can do, so the path is no longer what decides. An interpreter one-liner that cannot be PROVEN a pure read is now refused whatever it names. This is not a new doctrine, it is the one this file already states - the code is judged by an allowlist of what it may call - applied at the point that previously required a path rule to fire first. There is nothing left to obfuscate, because the filename stopped mattering. The cost is honest and worth naming: a legitimate `node -e` that writes any file is now refused and must use the Write tool, and a `node -e` whose require() argument is itself concatenated is refused one step earlier than before (it was already unverifiable). The reads this guard advertises still work: node -p of the manifest, a bound module ref used for a direct read, cat/jq/grep, and the Read tool. Twenty-three write shapes across five adversarial batteries are refused. 761 assertions, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt * fix(harness): let the plan un-ignore reach .claude/plans/completed Review found that `.claude/plans/*` plus `!.claude/plans/*.plan.md` cannot re-include a plan inside a subdirectory: git never descends into an excluded directory to evaluate a nested negation, so the `completed` entry was excluded whole. That is exactly where implement/SKILL.md files a finished plan (`mkdir -p .claude/plans/completed && mv ...`), and pr-review/SKILL.md reads it back, so a completed plan would silently never be committed - recreating the fresh-checkout-with-a-missing-contract failure the un-ignore was added to fix. Verified with git add --dry-run rather than check-ignore, whose exit code is ambiguous when the last matching rule is a negation: before, a top-level plan staged and .claude/plans/completed was reported ignored; after, both stage. A non-plan file in the same directory stays ignored, which is the half worth keeping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt * fix(harness): give PAUSED the same shape as the other four gate files Review found the best defect of this whole series, because it needs no cunning. PAUSED was the only one of the five path patterns without an optional directory prefix, and the only one tested against the whole raw command rather than per segment. So `cd .claude/manifests && touch PAUSED` matched nothing: the cd segment is a read, and the touch segment carries no directory text at all. That one command silently creates the human-only disarm switch, and the Stop gate exits immediately when it exists. Verified: four of five shapes were allowed, including `cd ... && echo x > PAUSED`. PAUSED now carries the same optional-prefix bare-name shape as signoff, surfaces and verdicts, and is judged per segment like them. Reads of it are allowed as a consequence, which also retires a false positive that blocked an ordinary `[ -f ... ]` existence test earlier in this session - a guard that refuses honest reads is one a future session disarms. The same review noted the sibling: plain shell adjacent-quote concatenation (`sign''off.json`, which the shell parses identically) split the literal for the other four. Empty quote pairs are now spliced out before path matching, next to the JS concatenation join. Also: WORKFLOW.md described /drive output as draft PRs in seven places, directly contradicting its own top table and the implement agent specs, which state that a draft PR silently skips reviewers. All seven now say ready-for-review. Five regression tests. 766 assertions, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pgh7M5YPWLyoDmZpWSTDLt --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
npx vitest runfrom apps/web - 1339 tests, 120 files, all passingnpx turbo run type-check- web + shared pass with 0 errors🤖 Generated with Claude Code