feat(adal): add AdaL harness integration - #614
Conversation
Adds AdaL (@sylphai/adal-cli) as a supported harness for live hooks. AdaL needs no event map and no canonicalizeEventType branch: its six lifecycle events (PreToolUse, UserPromptSubmit, PostToolUse, PostToolUseFailure, PermissionRequest, Stop) are already spelled as the canonical PascalCase HOOK_EVENT_TYPES, so it falls through the existing "already PascalCase, pass through" tail like claude does. Its settings file is Claude-shaped too, so the adal Integration mirrors claudeCode rather than adding a new writer. Only PreToolUse and UserPromptSubmit can prevent an action; the other four fire after the fact, so policies matching them are advisory. This is documented in CLAUDE.md so policy authors do not expect a post-action deny to block. Tool names are snake_case internally, so ADAL_TOOL_MAP maps them onto the Claude vocabulary (bash->Bash, read_file->Read, create_file->Write, replace_by_string->Edit) and ADAL_TOOL_INPUT_MAP maps create_file's new_string body onto content. Unknown names, including mcp__*, pass through unchanged, so existing builtin policies apply to AdaL sessions with no policy changes. Audit (Pillar 2) is inert: AdaL persists no replayable tool-event log yet, so src/audit/cli-adapters/adal.ts returns [] from both functions and documents why. It exists only because ADAPTERS is an exhaustive Record<IntegrationType, CliAdapter> — see the PR description for the question about making it Partial, mirroring INTEGRATIONS. The install-prompt menu test was updated because the value under test is the harness count itself (12 -> 13). Validation: bun run lint (0 errors), bunx tsc --noEmit clean, bun run build OK, 201 tests pass across the three touched suites, and the repo-wide failing-file set is unchanged from upstream/main.
📝 WalkthroughWalkthroughAdds AdaL as a supported CLI with hook installation, event and tool canonicalization, CLI metadata and command support, documentation, and an inert audit adapter because AdaL has no persisted replayable transcript. ChangesAdaL support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdaL
participant Settings
participant FailproofAI
AdaL->>Settings: Read .adal/settings.json
FailproofAI->>Settings: Install marked hook entries
AdaL->>FailproofAI: Send hook event
FailproofAI->>FailproofAI: Canonicalize tool and input
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/bin/valid-clis.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. bin/failproofai.mjsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/failproofai.mjs (1)
85-96: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd unit coverage for the new AdaL paths.
The supplied cohort adds behavior across CLI parsing, adapter registration, inert audit functions, and download resolution without corresponding tests under
__tests__/.
bin/failproofai.mjs#L85-L96: test--hook --cli adalplus install, uninstall, and single-policy CLI parsing.src/audit/cli-adapters/adal.ts#L36-L48: assert both inert functions return[].src/audit/cli-adapters/index.ts#L97-L103: assertgetAdapter("adal")returns the registered adapter.lib/download-session.ts#L155-L159: assert AdaL resolves tonullfor downloads.As per coding guidelines, every implementation change in
**/*.{ts,tsx,js,mjs}must have a corresponding unit test under__tests__/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/failproofai.mjs` around lines 85 - 96, Add unit tests under __tests__ for every AdaL change: in bin/failproofai.mjs, cover --hook --cli adal plus install, uninstall, and single-policy CLI parsing; in src/audit/cli-adapters/adal.ts, assert both inert functions return []; in src/audit/cli-adapters/index.ts, assert getAdapter("adal") returns the registered adapter; and in lib/download-session.ts, assert AdaL resolves to null for downloads.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/failproofai.mjs`:
- Line 328: Update the CLI validation used by the uninstall parser and the
policy add/remove parser to accept “adal” consistently with VALID_CLIS. Reuse
the canonical allowlist rather than maintaining separate lists, and ensure
related usage/help strings and command paths support both installing and
removing AdaL hooks.
In `@README.md`:
- Around line 119-123: The README supported-CLI grid currently exceeds the
documented six-column layout because the AdaL cell adds a seventh entry in the
second row. Update the surrounding table markup to keep each row at six columns,
placing AdaL on a new row or rebalancing the existing cells while preserving all
CLI entries.
---
Outside diff comments:
In `@bin/failproofai.mjs`:
- Around line 85-96: Add unit tests under __tests__ for every AdaL change: in
bin/failproofai.mjs, cover --hook --cli adal plus install, uninstall, and
single-policy CLI parsing; in src/audit/cli-adapters/adal.ts, assert both inert
functions return []; in src/audit/cli-adapters/index.ts, assert
getAdapter("adal") returns the registered adapter; and in
lib/download-session.ts, assert AdaL resolves to null for downloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d7d65b9-ffb8-4548-b260-be5c50fb22c5
⛔ Files ignored due to path filters (1)
assets/logos/adal.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
CLAUDE.mdREADME.md__tests__/hooks/adal-canonicalize.test.ts__tests__/hooks/install-prompt.test.ts__tests__/hooks/integrations.test.ts__tests__/lib/cli-registry.test.tsbin/failproofai.mjslib/cli-registry.tslib/download-session.tslib/projects.tssrc/audit/cli-adapters/adal.tssrc/audit/cli-adapters/index.tssrc/hooks/integrations.tssrc/hooks/tool-name-canonicalize.tssrc/hooks/types.ts
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
…lumns Addresses both CodeRabbit review comments. 1. AdaL was install-only. VALID_CLIS was duplicated once per subcommand parser and I had only updated the install copy, so `policies --uninstall --cli adal` and `policy add/remove --cli adal` silently rejected adal — users could install AdaL hooks but never remove them. Rather than adding adal to each copy, the allowlist is now a single module-scope VALID_CLIS shared by all three parsers. That removes the duplication that caused the drift in the first place. Verified: `policies --uninstall --cli adal --scope project` now reaches real logic instead of bailing out at arg parsing. Added __tests__/bin/valid-clis.test.ts to pin the invariant: exactly one declaration, covering every INTEGRATION_TYPES entry, consulted by every parser. Confirmed the test fails when a local copy is reintroduced. 2. The README supported-CLI grid documents a fixed six-per-row layout, and AdaL made row two seven cells wide. AdaL now starts a third row and the comment describes the fill-to-six rule instead of a hardcoded 2x6. Validation: lint 0 errors, tsc clean, build OK, 215 tests pass across the touched suites, repo-wide failing-file set unchanged from upstream/main.
|
Thanks @coderabbitai — both comments were correct and are fixed in e8bddf3. 1. AdaL was install-only — real bug, good catch. Rather than adding I also added 2. README grid. Right — the comment documents a fixed six-per-row layout and AdaL made row two seven cells wide. AdaL now starts a third row, and I reworded the comment to describe the fill-to-six rule rather than a hardcoded 2x6 (it would have gone stale at the next CLI regardless). On the "add unit coverage for the new AdaL paths" note: coverage was already in the original commit — Gate after the fix: The workflows are still showing |
|
Tip For best results, initiate chat on the files or code changes.
The pending 🐇✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/bin/valid-clis.test.ts`:
- Around line 25-29: Update the test case covering INTEGRATION_TYPES to assert
exact equality between VALID_CLIS and INTEGRATION_TYPES in both directions,
ensuring no unsupported CLI entries are accepted. Preserve the existing source
extraction and integration coverage while adding validation that every allowlist
entry is defined in INTEGRATION_TYPES.
- Around line 38-40: Update the --hook parser test around “accepts adal in the
--hook parser as well” to cover every supported integration, rather than
checking only the literal adal value. Prefer validating the parser through
VALID_CLIS.has(cliArg), or iterate over INTEGRATION_TYPES and assert each entry
is accepted, so additions to the supported CLI sets cannot be omitted from
explicit parser checks.
- Around line 32-35: Update the test “is consulted by every subcommand parser
that accepts --cli” to validate each parser independently rather than counting
all VALID_CLIS.has occurrences globally. Assert the allowlist check within the
install, uninstall, and policy add/remove parser blocks, or invoke each parser
with both valid and invalid CLI values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dfff9709-8ad7-42a4-81f0-b228fb9d9b77
📒 Files selected for processing (3)
README.md__tests__/bin/valid-clis.test.tsbin/failproofai.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- bin/failproofai.mjs
| it("covers every INTEGRATION_TYPES entry", () => { | ||
| const block = BIN_SOURCE.split("const VALID_CLIS = new Set(")[1]?.split("]);")[0] ?? ""; | ||
| for (const cli of INTEGRATION_TYPES) { | ||
| expect(block).toContain(`"${cli}"`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert exact allowlist equality.
This only checks that VALID_CLIS contains every integration, so unsupported extra entries can remain accepted without failing the test. Since the allowlist is documented as mirroring INTEGRATION_TYPES, compare the two sets in both directions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/bin/valid-clis.test.ts` around lines 25 - 29, Update the test case
covering INTEGRATION_TYPES to assert exact equality between VALID_CLIS and
INTEGRATION_TYPES in both directions, ensuring no unsupported CLI entries are
accepted. Preserve the existing source extraction and integration coverage while
adding validation that every allowlist entry is defined in INTEGRATION_TYPES.
| it("is consulted by every subcommand parser that accepts --cli", () => { | ||
| // install, uninstall, and policy add/remove each guard with VALID_CLIS.has. | ||
| const uses = BIN_SOURCE.match(/VALID_CLIS\.has\(/g) ?? []; | ||
| expect(uses.length).toBeGreaterThanOrEqual(3); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate each parser independently.
uses.length >= 3 can pass when multiple checks are in one parser and another --cli parser has no allowlist guard. Assert VALID_CLIS.has(...) within each install, uninstall, and policy parser block, or exercise each parser with valid and invalid CLI values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/bin/valid-clis.test.ts` around lines 32 - 35, Update the test “is
consulted by every subcommand parser that accepts --cli” to validate each parser
independently rather than counting all VALID_CLIS.has occurrences globally.
Assert the allowlist check within the install, uninstall, and policy add/remove
parser blocks, or invoke each parser with both valid and invalid CLI values.
| it("accepts adal in the --hook parser as well", () => { | ||
| expect(BIN_SOURCE).toContain('cliArg === "adal"'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover every integration in the --hook parser.
Checking only cliArg === "adal" will not catch a future CLI added to INTEGRATION_TYPES and VALID_CLIS but omitted from the hook parser’s explicit checks. Route this parser through VALID_CLIS.has(cliArg) or assert acceptance for every INTEGRATION_TYPES entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/bin/valid-clis.test.ts` around lines 38 - 40, Update the --hook
parser test around “accepts adal in the --hook parser as well” to cover every
supported integration, rather than checking only the literal adal value. Prefer
validating the parser through VALID_CLIS.has(cliArg), or iterate over
INTEGRATION_TYPES and assert each entry is accepted, so additions to the
supported CLI sets cannot be omitted from explicit parser checks.
2 participants
Description
Adds AdaL (
@sylphai/adal-cli) as a supported harness for live hooks (Pillar 1).AdaL is an open-source agentic coding CLI from SylphAI (the team behind AdalFlow), in the same category as Claude Code and Codex. Its hook contract is deliberately Claude-shaped, which makes this the smallest harness integration in the repo so far.
Why this is the minimal path
No event map, no
canonicalizeEventTypebranch. AdaL fires exactly six lifecycle events —PreToolUse,UserPromptSubmit,PostToolUse,PostToolUseFailure,PermissionRequest,Stop— and every one is already spelled identically to itsHOOK_EVENT_TYPESentry. So AdaL falls through the existing tail:An
ADAL_EVENT_MAPwould be an identity map, so I did not add one. A test pins this (every event AdaL fires is already a canonical HookEventType) so it fails loudly if AdaL ever diverges.Settings are Claude-shaped, so the
adalIntegration mirrorsclaudeCoderather than introducing a new writer:hooks: { <Event>: [{ matcher?, hooks: [{ type, command, timeout }] }] },timeoutin seconds, idempotent marker-based replacement.~/.adal/settings.json<cwd>/.adal/settings.jsonNo
localscope — AdaL has nosettings.local.jsonequivalent.Only two events can enforce.
PreToolUseandUserPromptSubmitrun before the action and can deny it. The other four fire after the fact, so a deny cannot reverse them — policies matching those events are advisory (same shape as Factory/Hermes on non-Stopevents). Documented inCLAUDE.mdso policy authors don't expect a post-action deny to block.Tool names are snake_case internally, so
ADAL_TOOL_MAPmaps them onto the Claude vocabulary (bash→Bash,read_file→Read,create_file→Write,replace_by_string→Edit, …) andADAL_TOOL_INPUT_MAPmapscreate_file'snew_stringbody ontocontent. Unknown names — includingmcp__*— pass through unchanged, so existing builtin policies apply to AdaL sessions with no policy changes.❓ One question: should
ADAPTERSbePartial, likeINTEGRATIONS?AdaL is hooks-only today. It does not yet persist a replayable tool-event log —
~/.adal/sessions/<id>/holds only side-artifacts, and~/.adal/adal.dbhas no tables — so there is no equivalent of Claude's JSONL transcript or Goose's SQLitemessagesrows.But
ADAPTERSinsrc/audit/cli-adapters/index.tsis an exhaustiveRecord<IntegrationType, CliAdapter>, so a live-hooks-only CLI cannot be registered without an audit adapter. I noticedINTEGRATIONSisPartialfor precisely the mirror reason:The inverse case — live hooks, no audit — has no such escape hatch.
So this PR ships
src/audit/cli-adapters/adal.tsas an inert adapter: both functions return[], with a header explaining exactly why and what to do when AdaL gains a transcript. NotablylistTranscriptsalso returns[]rather than enumerating sessions from the metadata files — listing sessions whose events can never be streamed would fill the dashboard with rows that always open empty, which reads as broken rather than unsupported.Happy to switch to making
ADAPTERSPartialinstead if you'd prefer that over an inert module — it's a smaller diff and arguably the more honest modelling. Your call; I went with the stub so this PR doesn't change shared architecture uninvited.Type of Change
Checklist
bun run lintpasses — 0 errors (5 pre-existing warnings in files I didn't touch)bunx tsc --noEmitpasses — cleanbun run test:run— no regressions: identical failing-file set toupstream/main, +1 passing file, +12 passing testsbun run buildsucceedsValidation notes
adal-canonicalize,integrations,cli-registry).bunx tsccaught three registries I'd otherwise have missed (ProjectCli,ADAPTERS,download-session's exhaustivenevercheck) — all handled.__tests__/hooks/install-prompt.test.tswas modified because the value under test is the harness count (12 → 13 CLIs, menu row totals and the undetected list). PerAGENTS.mdrule 5 this is the explicit exception — no assertion was weakened, only the expected counts/labels updated.adalis appended last in every registry so no existing harness ordering changes.main(git log --oneline origin/main ^HEADis empty).Out of scope (deliberately)
Session capture, dashboard history wiring, and an
integration-suitelive probe are all follow-up work — keeping this PR to hook install + canonicalization perCONTRIBUTING.md's "one concern per PR."Summary by CodeRabbit